Re: [RFC] Rebasing merges: a jorney to the ultimate solution(RoadClear)

[Sergey Organov]

Jacob Keller  writes:

> On Wed, Mar 28, 2018 at 4:29 AM, Sergey Organov  wrote:
>
>> Jacob Keller  writes:
>>
>> > On Tue, Mar 27, 2018 at 10:57 PM, Sergey Organov 
>> wrote:
>> >>
>> >> Hi Johannes,
>> >>
>> >> Johannes Schindelin  writes:
>> >> > Hi Sergey,
>> >> >
>> >>
>> >> [...]
>> >>
>> >> >> >> Reusing existing concepts where possible doesn`t have this
>> problem.
>> >> >> >
>> >> >> > Existing concepts are great. As long as they fit the requirements
>> of
>> >> >> > the new scenarios. In this case, `pick` does *not* fit the
>> > requirement
>> >> >> > of "rebase a merge commit".
>> >> >>
>> >> >> It does, provided you use suitable syntax.
>> >> >
>> >> > You know what `pick` would also do, provided you use suitable syntax?
>> > Pick
>> >> > your nose.
>> >> >
>> >> > Don't blame me for this ridiculous turn the discussion took.
>> >> >
>> >> > Of course, using the suitable syntax you can do anything. Unless there
>> > is
>> >> > *already* a syntax and you cannot break it for backwards-compatibility
>> >> > reasons, as is the case here.
>> >>
>> >> Backward compatibility to what? To a broken '--preserve-merges'? I had a
>> >> feel you've invented '--recreate-merges' exactly to break that
>> >> compatibility. No?
>> >>
>> >> Or is it "Backwards compatibility of a feature that existed only as a
>> >> topic branch in `next` before being worked on more?", as you say
>> >> yourself below?
>> >>
>> >
>> > I'm pretty sure he meant that changing the meaning and behavior of "pick"
>> > is incompatible, as people use scripts which check the edit lists, and
>> > these scripts would expect pick to behave in a certain way.
>>
>> Are we still speaking about that new --recreate-merges feature? You
>> already care for compatibility for it? You expect there are already
>> scripts that use it?
>>
>> Once again, it seems like you care and don't care about backward
>> compatibility at the same time, here is your phrase below:
>>
>> "He absolutely cares about compatibility, but in this case, the feature
>> has not yet been merged into an official release."
>>
>> Are we still speaking about that new --recreate-merges feature?
>>
>> Do you guys care for compatibility for this particular --recreate-merges
>> feature or not? I'm lost. "Yes" or "No" answer, if you please!
>>
>> -- Sergey
>>
>
> I care about the general compatibility of the rebase todo list regardless
> of which options you enabled on the command line to generate it.

It's a good thing in general, yes. However, I recall I was told by the
author that --recreate-merges was introduced exactly to break backward
compatibility of the todo list. If so, could we please agree to stop
using backward compatibility as an objection in the discussion of this
particular feature?

> Yes this has a bit of problem because *any* new todo command will
> break the todo list, but it's better to only add new commands rather
> than change semantics of existing ones.

I'm not against new commands in general. I'm against inventing new
entities without necessity, so, provided we did agree not to care about
compatibility, there should be some other necessity to invent yet
another command. I don't see such a necessity. Do you?

The main principle I stand for in this discussion though is that all the
commits should be treated equally as much as possible, new command or no
new command. Doing otherwise will lead to all kinds of troubles and
confusion, both in implementation and in user experience.

Overall, what I think is needed is extending the syntax of existing todo
commands to handle merge commits. This will give a provision to get it
right this time. Otherwise it will likely end up being yet another
subject of deprecation in the future.

-- Sergey

Re: Git Question

[Taylor Blau]

On Wed, Mar 28, 2018 at 08:49:19PM -0500, Mark Wartman wrote:
> I am following this tutorial and I expected to only see user.name &
> user.email, so what are the filters.lfs’s and the credential.helper?
> Should I ignore them, or try to get rid of them?  Please advise.

The `filter.lfs` configuration is set by Git LFS [1] in order to
correctly filter large files into your working copy.

You can safely ignore these, or remove them if you are not using Git
LFS.

> What is the best way to upgrade to a newer stable version of git?

On macOS, you can use Homebrew [2] to install any version of Git.
Install the latest via:

  brew update && brew install git

Thanks,
Taylor

[1]: https://git-lfs.github.com
[2]: https://brew.sh

Git Question

[Mark Wartman]

Hello,

I am currently running git version 2.10.1 (Apple Git-78).  When I run git 
config —list:   it returns
credential.helper-osxkeychain
filter.lfs.clean=git=lfs clean — %f
filter.lfs.smudge=git-lfs smudge — %f
filter.lfs.process=git-lfs filter-process
filter.lfs.required=true
user.name=markwartman1
user.email=markwart...@d2l.lonestar.edu

I am following this tutorial and I expected to only see user.name & user.email, 
so what are the filters.lfs’s and the credential.helper?  Should I ignore them, 
or try to get rid of them?  Please advise.  

What is the best way to upgrade to a newer stable version of git?

Thanks,

Mark

Re: [PATCH v2 4/4] builtin/config: introduce `--color` type specifier

[Taylor Blau]

On Mon, Mar 26, 2018 at 05:16:45AM -0400, Jeff King wrote:
> On Fri, Mar 23, 2018 at 08:55:56PM -0400, Taylor Blau wrote:
>
> > As of this commit, the canonical way to retreive an ANSI-compatible
> > color escape sequence from a configuration file is with the
> > `--get-color` action.
>
> s/retreive/retrieve/

Ack. I forgot to correct this in v3. I will remember to fix this in
anticipation of v4 before this is queued.

> > This is to allow Git to "fall back" on a default value for the color
> > should the given section not exist in the specified configuration(s).
> >
> > With the addition of `--default`, this is no longer needed since:
> >
> >   $ git config --default red --color core.section
> >
> > will be have exactly as:
> >
> >   $ git config --get-color core.section red
> >
> > For consistency, let's introduce `--color` and encourage `--color`,
> > `--default` together over `--get-color` alone.
>
> I don't think we'll ever get rid of --get-color (at the very least, we'd
> need a deprecation period). But it's probably worth adding a note under
> the --get-color description to mention that it's a historical synonym,
> and that using "--default" should be preferred.

I added a note in v3 under the `--get-color` section that it is now
considered "historical", and it is instead preferred to use

  $ git config --default= --type=color 

> > @@ -90,6 +91,7 @@ static struct option builtin_config_options[] = {
> > OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"), 
> > TYPE_BOOL_OR_INT),
> > OPT_BIT(0, "path", , N_("value is a path (file or directory 
> > name)"), TYPE_PATH),
> > OPT_BIT(0, "expiry-date", , N_("value is an expiry date"), 
> > TYPE_EXPIRY_DATE),
> > +   OPT_BIT(0, "color", , N_("value is a color"), TYPE_COLOR),
> > OPT_GROUP(N_("Other")),
> > OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
> > OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
>
> I just had a funny thought. Normally in Git the "--color" option means
> "colorize the output". And we are diverging from that here. I wonder if
> anybody would be confused by that, or if we would ever want to later add
> an option to colorize git-config output. Would we regret squatting on
> --color?
>
> I'm not sure what else to name it. Anything _except_ "--color" would
> diverge from the existing scheme of "--".
>
> If we were designing from scratch, I'd consider:
>
>   git config --type=int ...
>   git config --type=color ...
>
> etc. I'm not sure if it's worth trying to switch now (on the other hand,
> it resolves the documentation issue I mentioned earlier, since that
> would naturally group all of the types ;) ).
>
> It would be pretty easy to declare "--type" as the Right Way, and list
> "--int" as a historical synonym for "--type=int".

Agreed. I have posted a topic to the list containing a patch to the
above effect.

> > @@ -320,6 +327,12 @@ static char *normalize_value(const char *key, const 
> > char *value)
> > else
> > return xstrdup(v ? "true" : "false");
> > }
> > +   if (types == TYPE_COLOR) {
> > +   char v[COLOR_MAXLEN];
> > +   if (!git_config_color(v, key, value))
> > +   return xstrdup(value);
> > +   die("cannot parse color '%s'", value);
> > +   }
>
> Interesting. This doesn't actually normalize anything, since we always
> pass back the original value (or die). I think that's the right thing to
> do, since otherwise you'd end up with ANSI codes in your config file.
>
> I wondered at first if this should go in the "noop" normalization that
> TYPE_PATH undergoes. But I like that it actually sanity-checks the
> value. We should probably have a comment here explaining that yes, we
> parse and then throw away the value (similar to the one near TYPE_PATH).

That was my original intent, but I agree that this is confusing without
a comment explaining so. I have added such a comment in v3.

> I suspect that TYPE_EXPIRY_DATE should do the same thing (parse and
> complain if you fed nonsense, but always keep the original value).

Where?

> > +test_expect_success 'get --color' '
> > +   rm .git/config &&
> > +   git config foo.color "red" &&
> > +   git config --get --color foo.color | test_decode_color >actual &&
> > +   echo "" >expect &&
> > +   test_cmp expect actual
> > +'
>
> We should probably write this as:
>
>   git config --get --color foo.color >actual.raw &&
>   test_decode_color actual
>
> to catch failures from git-config itself (there's a lot of old tests
> which pipe, but we've been trying to convert them to be more careful).

Sounds good; I have added this in v3.

> > +test_expect_success 'set --color' '
> > +   rm .git/config &&
> > +   git config --color foo.color "red" &&
> > +   test_cmp expect .git/config
> > +'
> > +
> > +test_expect_success 'get --color barfs on non-color' '
> > +   echo "[foo]bar=not-a-color" >.git/config &&
> > +   test_must_fail git config --get 

Re: [PATCH v2 2/4] Documentation: list all type specifiers in config prose

[Taylor Blau]

On Mon, Mar 26, 2018 at 04:55:56AM -0400, Jeff King wrote:
> In fact, that kind of makes me wonder if this "type" list should not
> exist at all. If we instead grouped the options and said "these are the
> type options", then we'd only need one list.
>
> I'm OK to punt on that for now, though, in the interest of not holding
> up this patch series. I do think we should say something like:
>
>   Each type can be specified with the matching command-line option
>   (e.g., `--bool`, `--int`, etc); see <> below.

I punted on this for now, since rebasing on
tb/config-type-specifier-option makes this commit a no-op.


Thanks,
Taylor

Re: [PATCH v2 1/4] builtin/config: introduce `--default`

[Taylor Blau]

On Mon, Mar 26, 2018 at 04:34:42AM -0400, Jeff King wrote:
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > index e09ed5d7d..d9e389a33 100644
> > --- a/Documentation/git-config.txt
> > +++ b/Documentation/git-config.txt
> > @@ -233,8 +233,10 @@ See also <>.
> > using `--file`, `--global`, etc) and `on` when searching all
> > config files.
> >
> > -CONFIGURATION
> > --
> > +--default value::
> > +  When using `--get`, behave as if value were the value assigned to the 
> > given
> > +  slot.
> > +
> >  `pager.config` is only respected when listing configuration, i.e., when
> >  using `--list` or any of the `--get-*` which may return multiple results.
> >  The default is to use a pager.
>
> Hmm, what's going on with the CONFIGURATION header here?

My mistake, I have correct this erroneous diff in v3. Thanks for
pointing it out!

> For the description of "--default" itself, do we want to say something
> like:
>
>   When using `--get` and the requested slot is not found, behave as
>   if...
>
> That is hopefully a given from the name "--default", but it seems like
> an important point to mention.

Ditto.

> > +test_expect_success 'marshals default value as int' '
> > +   echo 810 >expect &&
> > +   git config --default 810 --int core.foo >actual &&
> > +   test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'marshals default value as int (storage unit)' '
> > +   echo 1048576 >expect &&
> > +   git config --default 1M --int core.foo >actual &&
> > +   test_cmp expect actual
> > +'
>
> I'm not sure what we're really testing in the first one. The storage
> unit conversion is the interesting bit.
>
> TBH, I'm not sure we need to separately test each possible type
> conversion here. If we know that the type conversions are tested
> elsewhere (and I would not be surprised if they're not, but let's assume
> for a moment...) and we check that type conversions are applied to
> "--default" values for some types, I don't think there's any reason to
> assume that the others would not work.

Agreed. I don't think that this test (and the ones that your comment
below concerns) are testing anything meaningful, or that would catch any
interesting future regressions. I have removed them from this series in
v3.


Thanks,
Taylor

Re: [PATCH v3 0/4] Teach `--default` to `git-config(1)`

[Taylor Blau]

On Wed, Mar 28, 2018 at 06:16:31PM -0700, Taylor Blau wrote:
> Attached is 'v3' of my patch series to add a `--default` option to `git 
> config`.

My apologies, this patch series is only 3 long, so the subject line of
this message is incorrect.


Thanks,
Taylor

[PATCH v3 2/3] config.c: introduce 'git_config_color' to parse ANSI colors

[Taylor Blau]

In preparation for adding `--color` to the `git-config(1)` builtin,
let's introduce a color parsing utility, `git_config_color` in a similar
fashion to `git_config_`.

Signed-off-by: Taylor Blau 
---
 config.c | 10 ++
 config.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/config.c b/config.c
index b0c20e6cb..33366b52c 100644
--- a/config.c
+++ b/config.c
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
struct config_source *prev;
@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const 
char *var, const char *
return 0;
 }
 
+int git_config_color(char *dest, const char *var, const char *value)
+{
+   if (!value)
+   return config_error_nonbool(var);
+   if (color_parse(value, dest) < 0)
+   return -1;
+   return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
/* This needs a better name */
diff --git a/config.h b/config.h
index ef70a9cac..0e060779d 100644
--- a/config.h
+++ b/config.h
@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
+extern int git_config_color(char *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const 
char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
-- 
2.16.2.440.gc6284da4f

[PATCH v3 3/3] builtin/config: introduce `color` type specifier

[Taylor Blau]

As of this commit, the canonical way to retreive an ANSI-compatible
color escape sequence from a configuration file is with the
`--get-color` action.

This is to allow Git to "fall back" on a default value for the color
should the given section not exist in the specified configuration(s).

With the addition of `--default`, this is no longer needed since:

  $ git config --default red --type=color core.section

will be have exactly as:

  $ git config --get-color core.section red

For consistency, let's introduce `--color` and encourage `--type=color`,
`--default` together over `--get-color` alone.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt | 11 +++
 builtin/config.c | 21 +
 t/t1300-repo-config.sh   | 30 ++
 3 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 5aa528878..aaba36615 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -38,10 +38,8 @@ existing values that match the regexp are updated or unset.  
If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <>).
 
-A type specifier may be given as an argument to `--type` to make 'git config'
-ensure that the variable(s) are of the given type and convert the value to the
-canonical form. If no type specifier is passed, no checks or transformations 
are
-performed on the value.
+`color`::
+The value is taken as an ANSI color escape sequence.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -177,6 +175,7 @@ Valid `[type]`'s include:
   ~/` from the command line to let your shell do the expansion.)
 - 'expiry-date': canonicalize by converting from a fixed or relative ate-string
   to a timestamp. This specifier has no effect when setting the value.
+- 'color': canonicalize by converting to an ANSI color escape sequence.
 +
 
 --bool::
@@ -184,6 +183,7 @@ Valid `[type]`'s include:
 --bool-or-int::
 --path::
 --expiry-date::
+--color::
   Historical options for selecting a type specifier. Prefer instead `--type`,
   (see: above).
 
@@ -223,6 +223,9 @@ Valid `[type]`'s include:
output it as the ANSI color escape sequence to the standard
output.  The optional `default` parameter is used instead, if
there is no color configured for `name`.
++
+It is preferred to use `--type=color`, or `--type=color --default=[default]`
+instead of `--get-color`.
 
 -e::
 --edit::
diff --git a/builtin/config.c b/builtin/config.c
index 4340f5f3d..1aae228a6 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -62,6 +62,7 @@ static int show_origin;
 #define TYPE_BOOL_OR_INT (1<<2)
 #define TYPE_PATH (1<<3)
 #define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_COLOR (1<<5)
 
 static struct option builtin_config_options[] = {
OPT_GROUP(N_("Config file location")),
@@ -177,6 +178,11 @@ static int format_config(struct strbuf *buf, const char 
*key_, const char *value
if (git_config_expiry_date(, key_, value_) < 0)
return -1;
strbuf_addf(buf, "%"PRItime, t);
+   } else if (types == TYPE_COLOR) {
+   char v[COLOR_MAXLEN];
+   if (git_config_color(v, key_, value_) < 0)
+   return -1;
+   strbuf_addstr(buf, v);
} else if (value_) {
strbuf_addstr(buf, value_);
} else {
@@ -322,6 +328,19 @@ static char *normalize_value(const char *key, const char 
*value)
else
return xstrdup(v ? "true" : "false");
}
+   if (types == TYPE_COLOR) {
+   char v[COLOR_MAXLEN];
+   if (!git_config_color(v, key, value))
+   /*
+* The contents of `v` now contain an ANSI escape
+* sequence, not suitable for including within a
+* configuration file. Treat the above as a
+* "sanity-check", and return the given value, which we
+* know is representable as valid color code.
+*/
+   return xstrdup(value);
+   die("cannot parse color '%s'", value);
+   }
 
die("BUG: cannot normalize type %d", types);
 }
@@ -519,6 +538,8 @@ static int type_name_to_specifier(char *name)
return TYPE_PATH;
else if (!(strcmp(name, "expiry-date")))
return TYPE_EXPIRY_DATE;
+   else if (!(strcmp(name, "color")))
+   return TYPE_COLOR;
die(_("unexpected --type argument, %s"), name);
 }
 
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 12dc94bd2..12e852a1d 100755
--- 

[PATCH v3 0/4] Teach `--default` to `git-config(1)`

[Taylor Blau]

Hi all,

Attached is 'v3' of my patch series to add a `--default` option to `git config`.

The most notable change since the last reroll is that this series is now based
upon tb/config-type-specifier-option [1], with discussion about that change here
[2]. This was motivated by my and Peff's shared concern that by adding `--color`
as a top-level type specifier, that we'd be "squatting" on adding `--color` in
the traditional sense of "colorize this output."

As such, `--color` is _not_ added as a top-level type specifier in this version,
rather, it is accessible via `--type=color`. This leaves `--color` unused and
available for other uses in the future.

Other changes since last time include:

  1. Fixing an erroneous diff in Documentation/git-config.txt, where a header
 was removed.

  2. `git config` will now die immediately when the `--default` value is not
  canonicalize able under the given type specifier.

  3. Removing redundancies from t1300 and t1310 that are--as Peff wisely points
 out--unlikely to catch any meaningful regressions in the future.

  4. Documenting behavior in builtin/config.c that we "throw away" the
 canonicalized value during normalization.

As always, thank you for the helpful feedback. I look forward to your thoughts
on this version of the series.


Thanks,
Taylor

Taylor Blau (3):
  builtin/config: introduce `--default`
  config.c: introduce 'git_config_color' to parse ANSI colors
  builtin/config: introduce `color` type specifier

 Documentation/git-config.txt | 15 ++
 builtin/config.c | 38 
 config.c | 10 ++
 config.h |  1 +
 t/t1300-repo-config.sh   | 30 
 t/t1310-config-default.sh| 38 
 6 files changed, 128 insertions(+), 4 deletions(-)
 create mode 100755 t/t1310-config-default.sh

--
2.16.2.440.gc6284da4f

[PATCH v3 1/3] builtin/config: introduce `--default`

[Taylor Blau]

For some use cases, callers of the `git-config(1)` builtin would like to
fallback to default values when the slot asked for does not exist. In
addition, users would like to use existing type specifiers to ensure
that values are parsed correctly when they do exist in the
configuration.

For example, to fetch a value without a type specifier and fallback to
`$fallback`, the following is required:

  $ git config core.foo || echo "$fallback"

This is fine for most values, but can be tricky for difficult-to-express
`$fallback`'s, like ANSI color codes.

This motivates `--get-color`, which is a one-off exception to the normal
type specifier rules wherein a user specifies both the configuration
slot and an optional fallback. Both are formatted according to their
type specifier, which eases the burden on the user to ensure that values
are correctly formatted.

This commit (and those following it in this series) aim to eventually
replace `--get-color` with a consistent alternative. By introducing
`--default`, we allow the `--get-color` action to be promoted to a
`--color` type specifier, retaining the "fallback" behavior via the
`--default` flag introduced in this commit.

For example, we aim to replace:

  $ git config --get-color slot [default] [...]

with:

  $ git config --default default --color slot [...]

Values filled by `--default` behave exactly as if they were present in
the affected configuration file; they will be parsed by type specifiers
without the knowledge that they are not themselves present in the
configuration.

Specifically, this means that the following will work:

  $ git config --int --default 1M does.not.exist
  1048576

In subsequent commits, we will offer `--color`, which (in conjunction
with `--default`) will be sufficient to replace `--get-color`.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt |  4 
 builtin/config.c | 17 
 t/t1310-config-default.sh| 38 
 3 files changed, 59 insertions(+)
 create mode 100755 t/t1310-config-default.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index a4a5ffb41..5aa528878 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -235,6 +235,10 @@ Valid `[type]`'s include:
using `--file`, `--global`, etc) and `on` when searching all
config files.
 
+--default value::
+  When using `--get`, and the requested slot is not found, behave as if value
+  were the value assigned to the that slot.
+
 CONFIGURATION
 -
 `pager.config` is only respected when listing configuration, i.e., when
diff --git a/builtin/config.c b/builtin/config.c
index ea7923493..4340f5f3d 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -27,6 +27,7 @@ static int use_global_config, use_system_config, 
use_local_config;
 static struct git_config_source given_config_source;
 static int actions, types;
 static char *type;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -96,6 +97,7 @@ static struct option builtin_config_options[] = {
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
OPT_BOOL(0, "includes", _includes_opt, N_("respect include 
directives on lookup")),
OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
(file, standard input, blob, command line)")),
+   OPT_STRING(0, "default", _value, N_("value"), N_("with --get, 
use default value when missing entry")),
OPT_END(),
 };
 
@@ -260,6 +262,16 @@ static int get_value(const char *key_, const char *regex_)
config_with_options(collect_config, ,
_config_source, _options);
 
+   if (!values.nr && default_value) {
+   struct strbuf *item;
+   ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+   item = [values.nr++];
+   strbuf_init(item, 0);
+   if (format_config(item, key_, default_value) < 0) {
+   exit(1);
+   }
+   }
+
ret = !values.nr;
 
for (i = 0; i < values.nr; i++) {
@@ -618,6 +630,11 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
usage_with_options(builtin_config_usage, 
builtin_config_options);
}
 
+   if (default_value && !(actions & ACTION_GET)) {
+   error("--default is only applicable to --get");
+   usage_with_options(builtin_config_usage, 
builtin_config_options);
+   }
+
if (type) {
if (types != 0) {
error("usage of --type is ambiguous");
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
new file mode 100755
index 0..ab4e35f06
--- /dev/null
+++ b/t/t1310-config-default.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='Test git config in different settings (with --default)'
+

[PATCH] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values are satisfiable under that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to add extend this functionality with
`--color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid inhabiting this option too soon, so that
`git config` can support `--color` (in the traditional sense) in the
future, if that is desired.

In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
`--int`, `--bool`, and etc. This allows the aforementioned other patch
to add `--color` (in the non-traditional sense) via `--type=color`,
instead of `--color`.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt | 66 +++-
 builtin/config.c | 25 ++
 t/t1300-repo-config.sh   | 21 
 3 files changed, 80 insertions(+), 32 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d..a4a5ffb41 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 
 [verse]
-'git config' [] [type] [--show-origin] [-z|--null] name [value 
[value_regex]]
-'git config' [] [type] --add name value
-'git config' [] [type] --replace-all name value [value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] --get name 
[value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] --get-all name 
[value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] [--name-only] 
--get-regexp name_regex [value_regex]
-'git config' [] [type] [-z|--null] --get-urlmatch name URL
+'git config' [] [--type] [--show-origin] [-z|--null] name [value 
[value_regex]]
+'git config' [] [--type] --add name value
+'git config' [] [--type] --replace-all name value [value_regex]
+'git config' [] [--type] [--show-origin] [-z|--null] --get name 
[value_regex]
+'git config' [] [--type] [--show-origin] [-z|--null] --get-all 
name [value_regex]
+'git config' [] [--type] [--show-origin] [-z|--null] 
[--name-only] --get-regexp name_regex [value_regex]
+'git config' [] [--type] [-z|--null] --get-urlmatch name URL
 'git config' [] --unset name [value_regex]
 'git config' [] --unset-all name [value_regex]
 'git config' [] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset. 
 If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+A type specifier may be given as an argument to `--type` to make 'git config'
+ensure that the variable(s) are of the given type and convert the value to the
+canonical form. If no type specifier is passed, no checks or transformations 
are
+performed on the value.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,34 @@ See also <>.
 --list::
List all variables set in config file, along with their values.
 
---bool::
-   'git config' will ensure that the output is "true" or "false"
+--type [type]::
+  'git config' will ensure that any input output is valid under the given type
+  constraint(s), and will canonicalize outgoing values in `[type]`'s canonical
+  form.
++
+Valid `[type]`'s include:
++
+- 'bool': canonicalize  values as either "true" or "false".
+- 'int': canonicalize  values as simple decimla numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 prior to output.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative ate-string
+  to a timestamp. This specifier has no effect when setting the value.
++
 
+--bool::
 --int::
-   'git config' will ensure that the output is a simple
- 

Re: Apparent bug in credential tool running...

[Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)]

Quick note: I did submit the patch, look for subject line " [PATCH] credential: 
cred helper fast exit can cause SIGPIPE, crash".

Thanks again Jeff,
Erik



Re: [PATCHv3 0/6] Moving submodules with nested submodules

[Jonathan Tan]

On Wed, 28 Mar 2018 15:35:25 -0700
Stefan Beller  wrote:

> v3:
> * reordered patches to have Jonathans patch before submodule_free
> * addressed Jonathans comments on patch 5.
> * rebased on origin/sb/object-store to resolve a visibility conflict
>   about repo_init being exposed outside of repository.c

All 6 patches look good to me, thanks.

Reviewed-by: Jonathan Tan 

Re: What's cooking in git.git (Mar 2018, #05; Wed, 28)

[Stefan Beller]

>> * sb/packfiles-in-repository (2018-03-26) 12 commits
> ...
>>
>>  (this branch uses nd/remove-ignore-env-field and sb/object-store.)
>>
>>  Refactoring of the internal global data structure continues.
>>
>>  Waiting for a follow-up discussion.
>
> I'll review this one more closely and start a discussion.

Actually this looks good to me.

[PATCHv3 2/6] grep: remove "repo" arg from non-supporting funcs

[Stefan Beller]

From: Jonathan Tan 

As part of commit f9ee2fcdfa ("grep: recurse in-process using 'struct
repository'", 2017-08-02), many functions in builtin/grep.c were
converted to also take "struct repository *" arguments. Among them were
grep_object() and grep_objects().

However, at least grep_objects() was converted incompletely - it calls
gitmodules_config_oid(), which references the_repository.

But it turns out that the conversion was extraneous anyway - there has
been no user-visible effect - because grep_objects() is never invoked
except with the_repository. This is because grepping through objects
cannot be done recursively into submodules.

Revert the changes to grep_objects() and grep_object() (which conversion
is also extraneous) to show that both these functions do not support
repositories other than the_repository.

Signed-off-by: Jonathan Tan 
Signed-off-by: Stefan Beller 
---
 builtin/grep.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 1e9cdbdf78..754eb6da3b 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -595,8 +595,7 @@ static int grep_tree(struct grep_opt *opt, const struct 
pathspec *pathspec,
 }
 
 static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
-  struct object *obj, const char *name, const char *path,
-  struct repository *repo)
+  struct object *obj, const char *name, const char *path)
 {
if (obj->type == OBJ_BLOB)
return grep_oid(opt, >oid, name, 0, path);
@@ -623,7 +622,7 @@ static int grep_object(struct grep_opt *opt, const struct 
pathspec *pathspec,
}
init_tree_desc(, data, size);
hit = grep_tree(opt, pathspec, , , base.len,
-   obj->type == OBJ_COMMIT, repo);
+   obj->type == OBJ_COMMIT, the_repository);
strbuf_release();
free(data);
return hit;
@@ -632,7 +631,6 @@ static int grep_object(struct grep_opt *opt, const struct 
pathspec *pathspec,
 }
 
 static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,
-   struct repository *repo,
const struct object_array *list)
 {
unsigned int i;
@@ -648,8 +646,8 @@ static int grep_objects(struct grep_opt *opt, const struct 
pathspec *pathspec,
submodule_free();
gitmodules_config_oid(_obj->oid);
}
-   if (grep_object(opt, pathspec, real_obj, list->objects[i].name, 
list->objects[i].path,
-   repo)) {
+   if (grep_object(opt, pathspec, real_obj, list->objects[i].name,
+   list->objects[i].path)) {
hit = 1;
if (opt->status_only)
break;
@@ -1098,7 +1096,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
if (cached)
die(_("both --cached and trees are given."));
 
-   hit = grep_objects(, , the_repository, );
+   hit = grep_objects(, , );
}
 
if (num_threads)
-- 
2.17.0.rc1.321.gba9d0f2565-goog

[PATCHv3 3/6] submodule-config: allow submodule_free to handle arbitrary repositories

[Stefan Beller]

At some point we may want to rename the function so that it describes what
it actually does as 'submodule_free' doesn't quite describe that this
clears a repository's submodule cache.  But that's beyond the scope of
this series.

While at it remove the extern key word from its declaration.

Signed-off-by: Stefan Beller 
---
 Documentation/technical/api-submodule-config.txt | 2 +-
 builtin/grep.c   | 2 +-
 submodule-config.c   | 6 +++---
 submodule-config.h   | 2 +-
 t/helper/test-submodule-config.c | 2 +-
 unpack-trees.c   | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/technical/api-submodule-config.txt 
b/Documentation/technical/api-submodule-config.txt
index 3dce003fda..44a85bbb8b 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -38,7 +38,7 @@ Data Structures
 Functions
 -
 
-`void submodule_free()`::
+`void submodule_free(struct repository *r)`::
 
Use these to free the internally cached values.
 
diff --git a/builtin/grep.c b/builtin/grep.c
index 754eb6da3b..c1f22fb9fb 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -643,7 +643,7 @@ static int grep_objects(struct grep_opt *opt, const struct 
pathspec *pathspec,
 
/* load the gitmodules file for this rev */
if (recurse_submodules) {
-   submodule_free();
+   submodule_free(the_repository);
gitmodules_config_oid(_obj->oid);
}
if (grep_object(opt, pathspec, real_obj, list->objects[i].name,
diff --git a/submodule-config.c b/submodule-config.c
index 2aa8a1747f..5b4f0baae8 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -642,8 +642,8 @@ const struct submodule *submodule_from_cache(struct 
repository *repo,
   key, lookup_path);
 }
 
-void submodule_free(void)
+void submodule_free(struct repository *r)
 {
-   if (the_repository->submodule_cache)
-   submodule_cache_clear(the_repository->submodule_cache);
+   if (r->submodule_cache)
+   submodule_cache_clear(r->submodule_cache);
 }
diff --git a/submodule-config.h b/submodule-config.h
index a5503a5d17..6b71a8cd30 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -46,6 +46,6 @@ extern const struct submodule *submodule_from_path(
 extern const struct submodule *submodule_from_cache(struct repository *repo,
const struct object_id 
*treeish_name,
const char *key);
-extern void submodule_free(void);
+void submodule_free(struct repository *r);
 
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c
index f23db3b19a..9971c5e9dd 100644
--- a/t/helper/test-submodule-config.c
+++ b/t/helper/test-submodule-config.c
@@ -64,7 +64,7 @@ int cmd_main(int argc, const char **argv)
arg += 2;
}
 
-   submodule_free();
+   submodule_free(the_repository);
 
return 0;
 }
diff --git a/unpack-trees.c b/unpack-trees.c
index e6a15bbe44..3a6a28e794 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -290,7 +290,7 @@ static void load_gitmodules_file(struct index_state *index,
if (!state && ce->ce_flags & CE_WT_REMOVE) {
repo_read_gitmodules(the_repository);
} else if (state && (ce->ce_flags & CE_UPDATE)) {
-   submodule_free();
+   submodule_free(the_repository);
checkout_entry(ce, state, NULL);
repo_read_gitmodules(the_repository);
}
-- 
2.17.0.rc1.321.gba9d0f2565-goog

[PATCHv3 4/6] submodule-config: add repository argument to submodule_from_{name, path}

[Stefan Beller]

This enables submodule_from_{name, path} to handle arbitrary repositories.
All callers just pass in the_repository, a later patch will pass in other
repos.

While at it remove the extern key word from the declarations.

Reviewed-by: Jonathan Tan 
Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c  | 14 +++---
 submodule-config.c   | 14 --
 submodule-config.h   | 10 ++
 submodule.c  | 30 --
 t/helper/test-submodule-config.c |  6 --
 5 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6d8e002be7..5551cf19c3 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -455,7 +455,7 @@ static void init_submodule(const char *path, const char 
*prefix,
 
displaypath = get_submodule_displaypath(path, prefix);
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(the_repository, _oid, path);
 
if (!sub)
die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -622,7 +622,7 @@ static void status_submodule(const char *path, const struct 
object_id *ce_oid,
struct rev_info rev;
int diff_files_result;
 
-   if (!submodule_from_path(_oid, path))
+   if (!submodule_from_path(the_repository, _oid, path))
die(_("no submodule mapping found in .gitmodules for path 
'%s'"),
  path);
 
@@ -742,7 +742,7 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
if (argc != 2)
usage(_("git submodule--helper name "));
 
-   sub = submodule_from_path(_oid, argv[1]);
+   sub = submodule_from_path(the_repository, _oid, argv[1]);
 
if (!sub)
die(_("no submodule mapping found in .gitmodules for path 
'%s'"),
@@ -773,7 +773,7 @@ static void sync_submodule(const char *path, const char 
*prefix,
if (!is_submodule_active(the_repository, path))
return;
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(the_repository, _oid, path);
 
if (sub && sub->url) {
if (starts_with_dot_dot_slash(sub->url) ||
@@ -926,7 +926,7 @@ static void deinit_submodule(const char *path, const char 
*prefix,
struct strbuf sb_config = STRBUF_INIT;
char *sub_git_dir = xstrfmt("%s/.git", path);
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(the_repository, _oid, path);
 
if (!sub || !sub->name)
goto cleanup;
@@ -1368,7 +1368,7 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
goto cleanup;
}
 
-   sub = submodule_from_path(_oid, ce->name);
+   sub = submodule_from_path(the_repository, _oid, ce->name);
 
if (suc->recursive_prefix)
displaypath = relative_path(suc->recursive_prefix,
@@ -1651,7 +1651,7 @@ static const char *remote_submodule_branch(const char 
*path)
const char *branch = NULL;
char *key;
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(the_repository, _oid, path);
if (!sub)
return NULL;
 
diff --git a/submodule-config.c b/submodule-config.c
index 5b4f0baae8..0ea1e927d2 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -619,18 +619,20 @@ static void gitmodules_read_check(struct repository *repo)
repo_read_gitmodules(repo);
 }
 
-const struct submodule *submodule_from_name(const struct object_id 
*treeish_name,
+const struct submodule *submodule_from_name(struct repository *r,
+   const struct object_id 
*treeish_name,
const char *name)
 {
-   gitmodules_read_check(the_repository);
-   return config_from(the_repository->submodule_cache, treeish_name, name, 
lookup_name);
+   gitmodules_read_check(r);
+   return config_from(r->submodule_cache, treeish_name, name, lookup_name);
 }
 
-const struct submodule *submodule_from_path(const struct object_id 
*treeish_name,
+const struct submodule *submodule_from_path(struct repository *r,
+   const struct object_id 
*treeish_name,
const char *path)
 {
-   gitmodules_read_check(the_repository);
-   return config_from(the_repository->submodule_cache, treeish_name, path, 
lookup_path);
+   gitmodules_read_check(r);
+   return config_from(r->submodule_cache, treeish_name, path, lookup_path);
 }
 
 const struct submodule *submodule_from_cache(struct repository *repo,
diff --git a/submodule-config.h b/submodule-config.h
index 6b71a8cd30..43dfe7dec0 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -39,10 +39,12 @@ extern int parse_update_recurse_submodules_arg(const char 
*opt, const 

[PATCHv3 0/6] Moving submodules with nested submodules

[Stefan Beller]

v3:
* reordered patches to have Jonathans patch before submodule_free
* addressed Jonathans comments on patch 5.
* rebased on origin/sb/object-store to resolve a visibility conflict
  about repo_init being exposed outside of repository.c

v2:
* addressed memleaks and messy code in patch 5
* removed the extern keyword where applicable
* extended the commit message, stating we want to rename submodule_free
  in the future.
* picked up Jonathans patch and added it as a nice finish of the series.
  I did not see the need or aesthetic desire to put that patch earlier
  in the series.
  
Thanks,
Stefan

v1:

This fixes the bug reported in [1] ("Bug: moving submodules that have submodules
inside them causes a fatal error in git status")

[1] https://public-inbox.org/git/20180306192017.ga5...@riseup.net/

Thanks,
Stefan
Jonathan Tan (1):
  grep: remove "repo" arg from non-supporting funcs

Stefan Beller (5):
  submodule.h: drop declaration of connect_work_tree_and_git_dir
  submodule-config: allow submodule_free to handle arbitrary
repositories
  submodule-config: add repository argument to submodule_from_{name,
path}
  submodule-config: remove submodule_from_cache
  submodule: fixup nested submodules after moving the submodule

 .../technical/api-submodule-config.txt|  2 +-
 builtin/grep.c| 14 ++---
 builtin/mv.c  |  6 +-
 builtin/submodule--helper.c   | 17 +++---
 dir.c | 60 ++-
 dir.h | 12 +++-
 repository.c  |  8 +--
 repository.h  |  3 +
 submodule-config.c| 29 -
 submodule-config.h| 15 +++--
 submodule.c   | 40 +++--
 submodule.h   |  1 -
 t/helper/test-submodule-config.c  |  8 ++-
 t/t7001-mv.sh |  2 +-
 unpack-trees.c|  2 +-
 15 files changed, 140 insertions(+), 79 deletions(-)

-- 
2.17.0.rc1.321.gba9d0f2565-goog

[PATCHv3 6/6] submodule: fixup nested submodules after moving the submodule

[Stefan Beller]

connect_work_tree_and_git_dir is used to connect a submodule worktree with
its git directory and vice versa after events that require a reconnection
such as moving around the working tree. As submodules can have nested
submodules themselves, we'd also want to fix the nested submodules when
asked to. Add an option to recurse into the nested submodules and connect
them as well.

As submodules are identified by their name (which determines their git
directory in relation to their superproject's git directory) internally
and by their path in the working tree of the superproject, we need to
make sure that the mapping of name <-> path is kept intact. We can do
that in the git-mv command by writing out the gitmodules file first
and then forcing a reload of the submodule config machinery.

Signed-off-by: Stefan Beller 
---
 builtin/mv.c|  6 ++--
 builtin/submodule--helper.c |  3 +-
 dir.c   | 60 +++--
 dir.h   | 12 +++-
 repository.c|  6 ++--
 repository.h|  3 ++
 submodule.c |  6 ++--
 t/t7001-mv.sh   |  2 +-
 8 files changed, 83 insertions(+), 15 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index cf3684d907..b0c5178e0d 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -275,10 +275,12 @@ int cmd_mv(int argc, const char **argv, const char 
*prefix)
die_errno(_("renaming '%s' failed"), src);
}
if (submodule_gitfile[i]) {
-   if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
-   connect_work_tree_and_git_dir(dst, 
submodule_gitfile[i]);
if (!update_path_in_gitmodules(src, dst))
gitmodules_modified = 1;
+   if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
+   connect_work_tree_and_git_dir(dst,
+ 
submodule_gitfile[i],
+ 1);
}
 
if (mode == WORKING_DIRECTORY)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5551cf19c3..ffdc51f426 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1260,8 +1260,7 @@ static int module_clone(int argc, const char **argv, 
const char *prefix)
strbuf_reset();
}
 
-   /* Connect module worktree and git dir */
-   connect_work_tree_and_git_dir(path, sm_gitdir);
+   connect_work_tree_and_git_dir(path, sm_gitdir, 0);
 
p = git_pathdup_submodule(path, "config");
if (!p)
diff --git a/dir.c b/dir.c
index ce6e50d2a2..4f401b6a3c 100644
--- a/dir.c
+++ b/dir.c
@@ -19,6 +19,7 @@
 #include "varint.h"
 #include "ewah/ewok.h"
 #include "fsmonitor.h"
+#include "submodule-config.h"
 
 /*
  * Tells read_directory_recursive how a file or directory should be treated.
@@ -2988,8 +2989,57 @@ void untracked_cache_add_to_index(struct index_state 
*istate,
untracked_cache_invalidate_path(istate, path);
 }
 
-/* Update gitfile and core.worktree setting to connect work tree and git dir */
-void connect_work_tree_and_git_dir(const char *work_tree_, const char 
*git_dir_)
+static void connect_wt_gitdir_in_nested(const char *sub_worktree,
+   const char *sub_gitdir)
+{
+   int i;
+   struct repository subrepo;
+   struct strbuf sub_wt = STRBUF_INIT;
+   struct strbuf sub_gd = STRBUF_INIT;
+
+   const struct submodule *sub;
+
+   /* If the submodule has no working tree, we can ignore it. */
+   if (repo_init(, sub_gitdir, sub_worktree))
+   return;
+
+   if (repo_read_index() < 0)
+   die("index file corrupt in repo %s", subrepo.gitdir);
+
+   for (i = 0; i < subrepo.index->cache_nr; i++) {
+   const struct cache_entry *ce = subrepo.index->cache[i];
+
+   if (!S_ISGITLINK(ce->ce_mode))
+   continue;
+
+   while (i + 1 < subrepo.index->cache_nr &&
+  !strcmp(ce->name, subrepo.index->cache[i + 1]->name))
+   /*
+* Skip entries with the same name in different stages
+* to make sure an entry is returned only once.
+*/
+   i++;
+
+   sub = submodule_from_path(, _oid, ce->name);
+   if (!sub || !is_submodule_active(, ce->name))
+   /* .gitmodules broken or inactive sub */
+   continue;
+
+   strbuf_reset(_wt);
+   strbuf_reset(_gd);
+   strbuf_addf(_wt, "%s/%s", sub_worktree, sub->path);
+   strbuf_addf(_gd, "%s/modules/%s", sub_gitdir, sub->name);
+
+   

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

[Junio C Hamano]

Stefan Beller  writes:

>> This looks nicer here in the script, but doesn't test exactly what users
>> type most of the time, I suppose.
>>
>> So how about this?
>
> Looks good to me, though I had a nagging feeling at first that the
> regex could be made more concise.
> Why do we need the optional "[^ ]" inside \1 ?
>
>> +   sed -e "s/^ \([^ ]* repo\) .*/-\1/" expect &&

At that position there's 40-hex object name.  If we want to go
looser, you could say

"s/^ \(.* repo\) .*/-\1/"

and if you want to go more strict, you could say

"s/^ \($_x40 repo\) (heads\/master)$/-\1/"

I think "Here between the leading SP and SP before the pathname
'repo', we expect an object name which should be a run of non SP
bytes" is a reasonable mid-point that is stricter than "anything
goes" and is still concise.

[PATCHv3 1/6] submodule.h: drop declaration of connect_work_tree_and_git_dir

[Stefan Beller]

The function connect_work_tree_and_git_dir is declared in both submodule.h
and dir.h, such that one of them is redundant. As the function is
implemented in dir.c, drop the declaration from submodule.h

Signed-off-by: Stefan Beller 
---
 submodule.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/submodule.h b/submodule.h
index b9b7ef0030..b6130e6287 100644
--- a/submodule.h
+++ b/submodule.h
@@ -105,7 +105,6 @@ extern int push_unpushed_submodules(struct oid_array 
*commits,
const char **refspec, int refspec_nr,
const struct string_list *push_options,
int dry_run);
-extern void connect_work_tree_and_git_dir(const char *work_tree, const char 
*git_dir);
 /*
  * Given a submodule path (as in the index), return the repository
  * path of that submodule in 'buf'. Return -1 on error or when the
-- 
2.17.0.rc1.321.gba9d0f2565-goog

[PATCHv3 5/6] submodule-config: remove submodule_from_cache

[Stefan Beller]

This continues the story of bf12fcdf5e (submodule-config: store
the_submodule_cache in the_repository, 2017-06-22).

The previous patch taught submodule_from_path to take a repository into
account, such that submodule_from_{path, cache} are the same now.
Remove submodule_from_cache, migrating all its callers to
submodule_from_path.

Reviewed-by: Jonathan Tan 
Signed-off-by: Stefan Beller 
---
 repository.c   | 2 +-
 submodule-config.c | 9 -
 submodule-config.h | 3 ---
 submodule.c| 4 ++--
 4 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/repository.c b/repository.c
index a4848c1bd0..eb5b8e9f5a 100644
--- a/repository.c
+++ b/repository.c
@@ -176,7 +176,7 @@ int repo_submodule_init(struct repository *submodule,
struct strbuf worktree = STRBUF_INIT;
int ret = 0;
 
-   sub = submodule_from_cache(superproject, _oid, path);
+   sub = submodule_from_path(superproject, _oid, path);
if (!sub) {
ret = -1;
goto out;
diff --git a/submodule-config.c b/submodule-config.c
index 0ea1e927d2..9abe9166d5 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -635,15 +635,6 @@ const struct submodule *submodule_from_path(struct 
repository *r,
return config_from(r->submodule_cache, treeish_name, path, lookup_path);
 }
 
-const struct submodule *submodule_from_cache(struct repository *repo,
-const struct object_id 
*treeish_name,
-const char *key)
-{
-   gitmodules_read_check(repo);
-   return config_from(repo->submodule_cache, treeish_name,
-  key, lookup_path);
-}
-
 void submodule_free(struct repository *r)
 {
if (r->submodule_cache)
diff --git a/submodule-config.h b/submodule-config.h
index 43dfe7dec0..6f686184e8 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -45,9 +45,6 @@ const struct submodule *submodule_from_name(struct repository 
*r,
 const struct submodule *submodule_from_path(struct repository *r,
const struct object_id 
*commit_or_tree,
const char *path);
-extern const struct submodule *submodule_from_cache(struct repository *repo,
-   const struct object_id 
*treeish_name,
-   const char *key);
 void submodule_free(struct repository *r);
 
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index 9279cff2d7..dac73d10a7 100644
--- a/submodule.c
+++ b/submodule.c
@@ -231,7 +231,7 @@ int is_submodule_active(struct repository *repo, const char 
*path)
const struct string_list *sl;
const struct submodule *module;
 
-   module = submodule_from_cache(repo, _oid, path);
+   module = submodule_from_path(repo, _oid, path);
 
/* early return if there isn't a path->module mapping */
if (!module)
@@ -1236,7 +1236,7 @@ static int get_next_submodule(struct child_process *cp,
if (!S_ISGITLINK(ce->ce_mode))
continue;
 
-   submodule = submodule_from_cache(spf->r, _oid, ce->name);
+   submodule = submodule_from_path(spf->r, _oid, ce->name);
if (!submodule) {
const char *name = default_name_or_path(ce->name);
if (name) {
-- 
2.17.0.rc1.321.gba9d0f2565-goog

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

[René Scharfe]

Am 28.03.2018 um 23:37 schrieb Stefan Beller:
>> This looks nicer here in the script, but doesn't test exactly what users
>> type most of the time, I suppose.
>>
>> So how about this?
> 
> Looks good to me, though I had a nagging feeling at first that the
> regex could be made more concise.
> Why do we need the optional "[^ ]" inside \1 ?
> 
>> +   sed -e "s/^ \([^ ]* repo\) .*/-\1/" expect &&

[:xdigit:] would match the hash more precisely, but I don't know how
widely this character class is supported among sed implementations.
And being so strict may require changes once SHA1 is replaced -- new
hashes might be marked with a special character.  And it's longer
anyway.

\S could be used instead, but I also don't know how widely this is
supported.

. could be used as well, of course, but that feels a bit sloppy to
me.

Using separate s commands for beginning and end of the line would
be shorter.  This here is slightly longer, anyway:

/ repo / {s/^ /-/; s/ (.*//}

What do you have in mind?  How to transform this:

 b2613938d15a18d9d4a504cacd9654fd1c879197 repo (heads/master)

... into this:

-b2613938d15a18d9d4a504cacd9654fd1c879197 repo

... while keeping other lines unchanged?

René

Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> This is what I got, which is slightly different from your series
> because I want to call set_git_dir() just one time (by
> setup_git_directory) and never again. I think the API looks close
> enough.
>
> I will probably rework on top of your chdir-notify instead (and let
> yours to be merged earlier)

I was occupied with the current cycle and did not have a chance to
read this one over before seeing this exchange.  After reading
Peff's chdir-notify thing, I agree with its general direction and
the idea to refine on top of it, which you two seem to have agreed.

Thanks for working well together.

[PATCH v4 4/5] stash: convert branch to builtin

[Joel Teichroeb]

Add stash branch to the helper and delete the apply_to_branch
function from the shell script.

Signed-off-by: Joel Teichroeb 
---
 builtin/stash--helper.c | 47 +++
 git-stash.sh| 17 ++---
 2 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 640c545f5..51fe8cab7 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -13,6 +13,7 @@
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
NULL
 };
@@ -27,6 +28,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_branch_usage[] = {
+   N_("git stash--helper branch  []"),
+   NULL
+};
+
 static const char * const git_stash_helper_clear_usage[] = {
N_("git stash--helper clear"),
NULL
@@ -509,6 +515,45 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+   const char *branch = NULL;
+   int ret;
+   struct argv_array args = ARGV_ARRAY_INIT;
+   struct stash_info info;
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_branch_usage, 0);
+
+   if (argc == 0)
+   return error(_("No branch name specified"));
+
+   branch = argv[0];
+
+   if (get_stash_info(, argc - 1, argv + 1))
+   return -1;
+
+   argv_array_pushl(, "checkout", "-b", NULL);
+   argv_array_push(, branch);
+   argv_array_push(, oid_to_hex(_commit));
+   ret = cmd_checkout(args.argc, args.argv, prefix);
+   if (ret) {
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_apply_stash(prefix, , 1);
+   if (!ret && info.is_stash_ref)
+   ret = do_drop_stash(prefix, );
+
+   free_stash_info();
+
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
int result = 0;
@@ -535,6 +580,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
result = clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
result = drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "branch"))
+   result = branch_stash(argc, argv, prefix);
else {
error(_("unknown subcommand: %s"), argv[0]);
usage_with_options(git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 0b8f07b38..c5fd4c6c4 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -598,20 +598,6 @@ drop_stash () {
clear_stash
 }
 
-apply_to_branch () {
-   test -n "$1" || die "$(gettext "No branch name specified")"
-   branch=$1
-   shift 1
-
-   set -- --index "$@"
-   assert_stash_like "$@"
-
-   git checkout -b $branch $REV^ &&
-   apply_stash "$@" && {
-   test -z "$IS_STASH_REF" || drop_stash "$@"
-   }
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -672,7 +658,8 @@ pop)
;;
 branch)
shift
-   apply_to_branch "$@"
+   cd "$START_DIR"
+   git stash--helper branch "$@"
;;
 *)
case $# in
-- 
2.16.2

[PATCH v4 5/5] stash: convert pop to builtin

[Joel Teichroeb]

Add stash pop to the helper and delete the pop_stash, drop_stash,
assert_stash_ref and pop_stash functions from the shell script
now that they are no longer needed.

Signed-off-by: Joel Teichroeb 
---
 builtin/stash--helper.c | 41 
 git-stash.sh| 50 -
 2 files changed, 45 insertions(+), 46 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 51fe8cab7..aa8a2bb3a 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,7 @@
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
+   N_("git stash--helper pop [--index] [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
@@ -23,6 +24,11 @@ static const char * const git_stash_helper_drop_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_pop_usage[] = {
+   N_("git stash--helper pop [--index] [-q|--quiet] []"),
+   NULL
+};
+
 static const char * const git_stash_helper_apply_usage[] = {
N_("git stash--helper apply [--index] [-q|--quiet] []"),
NULL
@@ -515,6 +521,39 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+   int index = 0, ret;
+   struct stash_info info;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_BOOL(0, "index", ,
+   N_("attempt to recreate the index")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_pop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   if (assert_stash_ref()) {
+   free_stash_info();
+   return -1;
+   }
+
+   if (do_apply_stash(prefix, , index)) {
+   printf_ln(_("The stash entry is kept in case you need it 
again."));
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_drop_stash(prefix, );
+   free_stash_info();
+   return ret;
+}
+
 static int branch_stash(int argc, const char **argv, const char *prefix)
 {
const char *branch = NULL;
@@ -580,6 +619,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
result = clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
result = drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "pop"))
+   result = pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
result = branch_stash(argc, argv, prefix);
else {
diff --git a/git-stash.sh b/git-stash.sh
index c5fd4c6c4..8f2640fe9 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -554,50 +554,6 @@ assert_stash_like() {
}
 }
 
-is_stash_ref() {
-   is_stash_like "$@" && test -n "$IS_STASH_REF"
-}
-
-assert_stash_ref() {
-   is_stash_ref "$@" || {
-   args="$*"
-   die "$(eval_gettext "'\$args' is not a stash reference")"
-   }
-}
-
-apply_stash () {
-   cd "$START_DIR"
-   git stash--helper apply "$@"
-   res=$?
-   cd_to_toplevel
-   return $res
-}
-
-pop_stash() {
-   assert_stash_ref "$@"
-
-   if apply_stash "$@"
-   then
-   drop_stash "$@"
-   else
-   status=$?
-   say "$(gettext "The stash entry is kept in case you need it 
again.")"
-   exit $status
-   fi
-}
-
-drop_stash () {
-   assert_stash_ref "$@"
-
-   git reflog delete --updateref --rewrite "${REV}" &&
-   say "$(eval_gettext "Dropped \${REV} (\$s)")" ||
-   die "$(eval_gettext "\${REV}: Could not drop stash entry")"
-
-   # clear_stash if we just dropped the last stash entry
-   git rev-parse --verify --quiet "$ref_stash@{0}" >/dev/null ||
-   clear_stash
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -634,7 +590,8 @@ push)
;;
 apply)
shift
-   apply_stash "$@"
+   cd "$START_DIR"
+   git stash--helper apply "$@"
;;
 clear)
shift
@@ -654,7 +611,8 @@ drop)
;;
 pop)
shift
-   pop_stash "$@"
+   cd "$START_DIR"
+   git stash--helper pop "$@"
;;
 branch)
shift
-- 
2.16.2

[PATCH v4 2/5] stash: convert apply to builtin

[Joel Teichroeb]

Add a bulitin helper for performing stash commands. Converting
all at once proved hard to review, so starting with just apply
let conversion get started without the other command being
finished.

The helper is being implemented as a drop in replacement for
stash so that when it is complete it can simply be renamed and
the shell script deleted.

Delete the contents of the apply_stash shell function and replace
it with a call to stash--helper apply until pop is also
converted.

Signed-off-by: Joel Teichroeb 
---
 .gitignore  |   1 +
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 444 
 git-stash.sh|  75 +---
 git.c   |   1 +
 6 files changed, 453 insertions(+), 70 deletions(-)
 create mode 100644 builtin/stash--helper.c

diff --git a/.gitignore b/.gitignore
index 833ef3b0b..296d5f376 100644
--- a/.gitignore
+++ b/.gitignore
@@ -152,6 +152,7 @@
 /git-show-ref
 /git-stage
 /git-stash
+/git-stash--helper
 /git-status
 /git-stripspace
 /git-submodule
diff --git a/Makefile b/Makefile
index 96f6138f6..6cfdbe9a3 100644
--- a/Makefile
+++ b/Makefile
@@ -1028,6 +1028,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 42378f3aa..a14fd85b0 100644
--- a/builtin.h
+++ b/builtin.h
@@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, const 
char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 0..00e854734
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,444 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "lockfile.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+#include "dir.h"
+
+static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_helper_apply_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static int quiet;
+static char stash_index_path[PATH_MAX];
+
+struct stash_info {
+   struct object_id w_commit;
+   struct object_id b_commit;
+   struct object_id i_commit;
+   struct object_id u_commit;
+   struct object_id w_tree;
+   struct object_id b_tree;
+   struct object_id i_tree;
+   struct object_id u_tree;
+   struct strbuf revision;
+   int is_stash_ref;
+   int has_u;
+};
+
+static int get_symbolic_name(const char *symbolic, struct strbuf *out)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "rev-parse", "--symbolic-full-name", NULL);
+   argv_array_push(, symbolic);
+   return pipe_command(, NULL, 0, out, 0, NULL, 0);
+}
+
+static int have_stash(void)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   cp.git_cmd = 1;
+   cp.no_stdout = 1;
+   argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+   argv_array_push(, ref_stash);
+   return pipe_command(, NULL, 0, NULL, 0, NULL, 0);
+}
+
+static void free_stash_info(struct stash_info *info)
+{
+   strbuf_release(>revision);
+}
+
+static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+{
+   struct strbuf w_commit_rev = STRBUF_INIT;
+   struct strbuf b_commit_rev = STRBUF_INIT;
+   struct strbuf w_tree_rev = STRBUF_INIT;
+   struct strbuf b_tree_rev = STRBUF_INIT;
+   struct strbuf i_tree_rev = STRBUF_INIT;
+   struct strbuf u_tree_rev = STRBUF_INIT;
+   struct strbuf symbolic = STRBUF_INIT;
+   struct strbuf out = STRBUF_INIT;
+   int ret;
+   const char *revision;
+   const char *commit = NULL;
+   char *end_of_rev;
+   info->is_stash_ref = 0;
+
+   if (argc > 1) {
+   int i;
+   struct strbuf refs_msg = STRBUF_INIT;
+   for (i = 0; i < argc; ++i)
+   

[PATCH v4 0/5] Convert some stash functionality to a builtin

[Joel Teichroeb]

I've been working on converting all of git stash to be a
builtin, however it's hard to get it all working at once with
limited time, so I've moved around half of it to a new
stash--helper builtin and called these functions from the shell
script. Once this is stabalized, it should be easier to convert
the rest of the commands one at a time without breaking
anything.

I've sent most of this code before, but that was targetting a
full replacement of stash. The code is overall the same, but
with some code review changes and updates for internal api
changes.

Since there seems to be interest from GSOC students who want to
work on converting builtins, I figured I should finish what I
have that works now so they could build on top of it.

The code is based on next as write_cache_as_tree was changed to
take an object ID. It can easily be rebase on master by changing
the two calls to write_cache_as_tree to use tha hash.

The only comments on v3 were minor, so I feel this should be
ready to go in soon.

Previous threads:
v1: https://public-inbox.org/git/20180325173916.GE10909@hank/T/
v2: https://public-inbox.org/git/20180326011426.19159-1-j...@teichroeb.net/
v3: https://public-inbox.org/git/20180327054432.26419-1-j...@teichroeb.net/

Changes from v3:
 - Fix the Windows build
 - Fix a use after free in the error handling

Joel Teichroeb (5):
  stash: improve option parsing test coverage
  stash: convert apply to builtin
  stash: convert drop and clear to builtin
  stash: convert branch to builtin
  stash: convert pop to builtin

 .gitignore  |   1 +
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 633 
 git-stash.sh| 136 +--
 git.c   |   1 +
 t/t3903-stash.sh|  16 ++
 7 files changed, 661 insertions(+), 128 deletions(-)
 create mode 100644 builtin/stash--helper.c

-- 
2.16.2

[PATCH v4 1/5] stash: improve option parsing test coverage

[Joel Teichroeb]

In preparation for converting the stash command incrementally to
a builtin command, this patch improves test coverage of the option
parsing. Both for having too many paramerters, or too few.

Signed-off-by: Joel Teichroeb 
---
 t/t3903-stash.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index aefde7b17..8a666c60c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -84,6 +84,17 @@ test_expect_success 'apply stashed changes (including 
index)' '
test 1 = $(git show HEAD:file)
 '
 
+test_expect_success 'giving too many ref agruments does nothing' '
+
+   for type in apply drop pop show "branch stash-branch"
+   do
+   test-chmtime =123456789 file &&
+   test_must_fail git stash $type stash@{0} stash@{1} 2>err &&
+   test_i18ngrep "Too many" err &&
+   test 123456789 = $(test-chmtime -v +0 file | sed 
's/[^0-9].*$//') || return 1
+   done
+'
+
 test_expect_success 'unstashing in a subdirectory' '
git reset --hard HEAD &&
mkdir subdir &&
@@ -479,6 +490,11 @@ test_expect_success 'stash branch - stashes on stack, 
stash-like argument' '
test $(git ls-files --modified | wc -l) -eq 1
 '
 
+test_expect_success 'stash branch complains with no arguments' '
+   test_must_fail git stash branch 2>err &&
+   test_i18ngrep "No branch name specified" err
+'
+
 test_expect_success 'stash show format defaults to --stat' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
-- 
2.16.2

[PATCH] credential: cred helper fast exit can cause SIGPIPE, crash

[Erik E Brady]

credential.c, run_credential_helper(): now ignores SIGPIPE
when writing to credential helper.  Avoids problem with race
where cred helper exits very quickly and, after, git tries
to write to it, generating SIGPIPE and crashing git.  To
reproduce this the cred helper must not read from STDIN.

This was seen with a custom credential helper, written in
Go, which ignored the store command (STDIN not read) and
then did a quick exit.  Even with this fast helper the race
was pretty rare, ie: was only seen on some of our older VM's
running 2.6.18-416.el5 #1 SMP linux for whatever reason.  On
these VM's it occurred only once every few hundred git cmds.
---
 credential.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/credential.c b/credential.c
index 9747f47b1..62be651b0 100644
--- a/credential.c
+++ b/credential.c
@@ -5,6 +5,7 @@
 #include "run-command.h"
 #include "url.h"
 #include "prompt.h"
+#include "sigchain.h"
 
 void credential_init(struct credential *c)
 {
@@ -227,8 +228,10 @@ static int run_credential_helper(struct credential *c,
return -1;
 
fp = xfdopen(helper.in, "w");
+   sigchain_push(SIGPIPE, SIG_IGN);
credential_write(c, fp);
fclose(fp);
+   sigchain_pop(SIGPIPE);
 
if (want_output) {
int r;
-- 
2.16.3.dirty

[PATCH v4 3/5] stash: convert drop and clear to builtin

[Joel Teichroeb]

Add the drop and clear commands to the builtin helper. These two
are each simple, but are being added together as they are quite
related.

We have to unfortunately keep the drop and clear functions in the
shell script as functions are called with parameters internally
that are not valid when the commands are called externally. Once
pop is converted they can both be removed.

Signed-off-by: Joel Teichroeb 
---
 builtin/stash--helper.c | 101 
 git-stash.sh|   4 +-
 2 files changed, 103 insertions(+), 2 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 00e854734..640c545f5 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -11,7 +11,14 @@
 #include "dir.h"
 
 static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper clear"),
+   NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
NULL
 };
 
@@ -20,6 +27,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_clear_usage[] = {
+   N_("git stash--helper clear"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static char stash_index_path[PATH_MAX];
@@ -168,6 +180,29 @@ static int get_stash_info(struct stash_info *info, int 
argc, const char **argv)
return 0;
 }
 
+static int do_clear_stash(void)
+{
+   struct object_id obj;
+   if (get_oid(ref_stash, ))
+   return 0;
+
+   return delete_ref(NULL, ref_stash, , 0);
+}
+
+static int clear_stash(int argc, const char **argv, const char *prefix)
+{
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options, 
git_stash_helper_clear_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+   if (argc != 0)
+   return error(_("git stash--helper clear with parameters is 
unimplemented"));
+
+   return do_clear_stash();
+}
+
 static int reset_tree(struct object_id *i_tree, int update, int reset)
 {
struct unpack_trees_options opts;
@@ -412,6 +447,68 @@ static int apply_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int do_drop_stash(const char *prefix, struct stash_info *info)
+{
+   struct argv_array args = ARGV_ARRAY_INIT;
+   int ret;
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   argv_array_pushl(, "reflog", "delete", "--updateref", "--rewrite", 
NULL);
+   argv_array_push(, info->revision.buf);
+   ret = cmd_reflog(args.argc, args.argv, prefix);
+   if (!ret) {
+   if (!quiet)
+   printf(_("Dropped %s (%s)\n"), info->revision.buf, 
oid_to_hex(>w_commit));
+   } else {
+   return error(_("%s: Could not drop stash entry"), 
info->revision.buf);
+   }
+
+   cp.git_cmd = 1;
+   /* Even though --quiet is specified, rev-parse still outputs the hash */
+   cp.no_stdout = 1;
+   argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+   argv_array_pushf(, "%s@{0}", ref_stash);
+   ret = run_command();
+
+   if (ret)
+   do_clear_stash();
+
+   return 0;
+}
+
+static int assert_stash_ref(struct stash_info *info)
+{
+   if (!info->is_stash_ref)
+   return error(_("'%s' is not a stash reference"), 
info->revision.buf);
+
+   return 0;
+}
+
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+   struct stash_info info;
+   int ret;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_drop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   if (assert_stash_ref()) {
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_drop_stash(prefix, );
+   free_stash_info();
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
int result = 0;
@@ -434,6 +531,10 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
usage_with_options(git_stash_helper_usage, options);
else if (!strcmp(argv[0], "apply"))
result = apply_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "clear"))
+   result = clear_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "drop"))
+   result = drop_stash(argc, argv, prefix);
else {
error(_("unknown subcommand: %s"), argv[0]);

Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Junio C Hamano]

Jonathan Nieder  writes:

> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,
> 2017-12-08), it was guarded by the uploadpack.allowFilter config item
> to allow server operators to control when they start supporting it.
>
> That config item didn't go far enough, though: it controls whether the
> 'filter' capability is advertised, but if a (custom) client ignores
> the capability advertisement and passes a filter specification anyway,
> the server would handle that despite allowFilter being false.
>
> This is particularly significant if a security bug is discovered in
> this new experimental partial clone code.  Installations without
> uploadpack.allowFilter ought not to be affected since they don't
> intend to support partial clone, but they would be swept up into being
> vulnerable.
>
> Simplify and limit the attack surface by making uploadpack.allowFilter
> disable the feature, not just the advertisement of it.
>
> NEEDSWORK: tests
>
> Signed-off-by: Jonathan Nieder 
> ---
> Noticed while reviewing the corresponding JGit code.
>
> If this change seems like a good idea, I can add tests and re-send it
> for real.

Yup.  The names of the static variables tell almost the whole story
to convince me why this is a good change ;-).  It is not about
advertising a feature alone, but if the feature is actually enabled,
so advertisement and handling of the feature should be either both
enabled or disabled.

Thanks.

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

[Stefan Beller]

> This looks nicer here in the script, but doesn't test exactly what users
> type most of the time, I suppose.
>
> So how about this?

Looks good to me, though I had a nagging feeling at first that the
regex could be made more concise.
Why do we need the optional "[^ ]" inside \1 ?

> +   sed -e "s/^ \([^ ]* repo\) .*/-\1/" expect &&

Re: git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec

[Stefan Beller]

On Wed, Mar 28, 2018 at 12:37 PM, Peter Oberndorfer  wrote:

>>> 2) Should "git submodule deinit" work on submodules that were removed by
>> upstream already?
>>
>> To answer the question "Is this a submodule that upstream removed
>> (recently)?"
>> we'd have to put in some effort, essentially checking if that was ever a
>> submodule
>> (and not a directory or file).
>>
>
> Hmm, yeah looks a bit more complicated than I initially imagined
> since submodules can have a name that's different from their path.
> And after the rebase, the name <-> path mapping via .gitmodules is not 
> available anymore.
>
> Naively I think it could work the following way:
> * Either iterate over all submodules in .git/modules/ and check their config
>   has a worktree = "../../path" that resolves to the submodule path we want 
> to remove.

This would work but scales linearly with the number of submodules.


> * Or check the "gitlink:" path in submodule/.git if it points to our 
> .git/modules/
> Then if .git/config contains a [submodule "name"] entry
> we should have a pretty good idea if this folder contains a stale submodule.

If you move a submodule a directory up or down, the relative path is not exact
any more, we'd need to check for the last part to loosely match.


>> When using "git pull --recurse-submodules" the submodule ought to be removed
>> automatically.
>>
>> When doing a fetch && merge manually, we may want to teach merge to remove
>> a submodule that we have locally upon merge, too.
>>
>
> Yeah that would be nice :-)
> In my case I updated the repository via a rebase, so that would also have to 
> be covered.

Oh rebase itself has not yet learned about recursion into submodules.
("git pull --rebase --recurse-submodules" is a thing though)

>> I view the git-submodule command as a bare bones plumbing helper, that we'd
>> want
>> to deprecate eventually as all other higher level commands will know how to
>> deal
>> with submodules.
>>
>> So I think we do not want to teach "git submodule deinit" to remove dormant
>> repositories, that were submodules removed by upstream already.
>>
>
> My gut feeling makes me expect the following:
> * It would be nice if such stale submodules showed up in "git submodule 
> status" or "git status"
>   Now "git submodule" shows nothing related to this stale submodule

That has currently only two ways "+" or "-" for there/not there.
Maybe we'd need to add some characters similar to "git status --porcelain"
such as "?"

>   Now "git status" shows  Untracked files: src/rt which is a bit confusing as 
> the actual submodule is in src/rt/hoedown
>   Now "Git gui" shows src/rt/hoedown as untracked git repository

hm. The current state of affairs doesn't sound intriguing.
Though, I think we'd want to step back one more step and rather want
to ask how a dormant submodule comes into existence, instead of
just improving the reporting. Reportingthem is of course also important,
but in the long run I'd rather want to have situations like these happen
less often. When upstream deletes a file, they are also not required to be
deleted manually, but merge/checkout would take care of them.

> * There should be an easy(and safe) way for the user to deinit such a 
> submodule
>   if if the automatic submodule updating during a merge/rebase was not 
> enabled or somehow failed.
> (Minus the problem of somebody having to actually do the work...)
>
>>> ~/src/rust/rust$ git submodule status
>> ...
>>>   b87873eaceb75cf9342d5273f01ba2c020f61ca8 src/tools/lld ((null))
>>
>>> -> strangely I get (null) for the current branch/commit in some
>> submodules?
>>
>> This sounds like (3). Looking into that.
>
> Sorry, what do you mean by (3)?

I meant the ((null)) issue is another third thought that we can
discuss separately,
slightly unrelated to the others (that you marked as (1) and (2))

Thanks,
Stefan

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

[René Scharfe]

Am 28.03.2018 um 22:21 schrieb Eric Sunshine:
> On Wed, Mar 28, 2018 at 4:08 PM, Stefan Beller  wrote:
>> On Wed, Mar 28, 2018 at 11:57 AM, Eric Sunshine  
>> wrote:
 +test_expect_success 'moving the submodule does not break the 
 superproject' '
 +   (
 +   cd addtest2 &&
 +
 +   mv repo repo.bak &&
 +   git submodule status >actual &&
 +   grep -e "^-" -e repo actual &&
 +
 +   mv repo.bak repo
>>>
>>> Should this "move back" be encapsulated in a test_when_finished?
>>
>> I thought about that, but decided against it for some reason as I was 
>> debating
>> where to put the test_when_finished. I mostly saw those at the very beginning
>> of a test and wondered if it can be called from within a subshell.
>> (I'd not want to put it at the beginning but rather adjacent to the move.)
> 
> It looks like test_when_finished() shouldn't be used in a subshell.
> However, wouldn't the following be reasonable?
> 
>  mv addtest2/repo addtest2/repo.bak &&
>  test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
>  (
>  cd addtest2 &&
>  git submodule status >actual &&
>  grep -e "^-" -e repo actual
>  )

I like this version, except for the grep call -- it accepts either lines
starting with a dash or containing "repo".  So if status would report
just "-" and nothing else then the test would pass. 

> Or, even simpler:
> 
>  mv addtest2/repo addtest2/repo.bak &&
>  test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
>  git -C addtest2 submodule status >actual &&
>  grep -e "^-" -e repo actual
> 

This looks nicer here in the script, but doesn't test exactly what users
type most of the time, I suppose.

So how about this?

-- >8 --
Subject: [PATCH v3] submodule: check for NULL return of 
get_submodule_ref_store()

If we can't find a ref store for a submodule then assume the latter
is not initialized (or was removed).  Print a status line accordingly
instead of causing a segmentation fault by passing NULL as the first
parameter of refs_head_ref().

Reported-by: Jeremy Feusi 
Reviewed-by: Stefan Beller 
Initial-Test-By: Stefan Beller 
Helped-by: Eric Sunshine 
Signed-off-by: Rene Scharfe 
---
 builtin/submodule--helper.c |  8 ++--
 t/t7400-submodule-basic.sh  | 15 +++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6ba8587b6d..9a0fb5e784 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -654,9 +654,13 @@ static void status_submodule(const char *path, const 
struct object_id *ce_oid,
 displaypath);
} else if (!(flags & OPT_CACHED)) {
struct object_id oid;
+   struct ref_store *refs = get_submodule_ref_store(path);
 
-   if (refs_head_ref(get_submodule_ref_store(path),
- handle_submodule_head_ref, ))
+   if (!refs) {
+   print_status(flags, '-', path, ce_oid, displaypath);
+   goto cleanup;
+   }
+   if (refs_head_ref(refs, handle_submodule_head_ref, ))
die(_("could not resolve HEAD ref inside the "
  "submodule '%s'"), path);
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index a39e69a3eb..ef1ea8d6b0 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -821,6 +821,21 @@ test_expect_success 'moving the superproject does not 
break submodules' '
)
 '
 
+test_expect_success 'moving the submodule does not break the superproject' '
+   (
+   cd addtest2 &&
+   git submodule status
+   ) >actual &&
+   sed -e "s/^ \([^ ]* repo\) .*/-\1/" expect &&
+   mv addtest2/repo addtest2/repo.bak &&
+   test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
+   (
+   cd addtest2 &&
+   git submodule status
+   ) >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'submodule add --name allows to replace a submodule with 
another at the same path' '
(
cd addtest2 &&
-- 
2.16.3

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

[Stefan Beller]

On Wed, Mar 28, 2018 at 1:21 PM, Eric Sunshine  wrote:

> Or, even simpler:
>
> mv addtest2/repo addtest2/repo.bak &&
> test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
> git -C addtest2 submodule status >actual &&
> grep -e "^-" -e repo actual

I like this!
Time permitting I'll update the patch (I'll first have to take
care of some other series, so feel free to make it into a patch)

Thanks,
Stefan

Re: What's cooking in git.git (Mar 2018, #05; Wed, 28)

[Stefan Beller]

On Wed, Mar 28, 2018 at 12:58 PM, Junio C Hamano  wrote:
>
> * ab/doc-hash-brokenness (2018-03-27) 2 commits
>  - doc hash-function-transition: clarify what SHAttered means
>  - doc hash-function-transition: clarify how older gits die on NewHash

This looked ready for next to me.

>
> * jk/branch-l-0-deprecation (2018-03-26) 3 commits
>  - branch: deprecate "-l" option
>  - t: switch "branch -l" to "branch --create-reflog"
>  - t3200: unset core.logallrefupdates when testing reflog creation
>  (this branch is used by jk/branch-l-1-removal and 
> jk/branch-l-2-reincarnation.)
>
>
> * jk/branch-l-1-removal (2018-03-26) 1 commit
>  - branch: drop deprecated "-l" option
>  (this branch is used by jk/branch-l-2-reincarnation; uses 
> jk/branch-l-0-deprecation.)
>
>
> * jk/branch-l-2-reincarnation (2018-03-26) 1 commit
>  - branch: make "-l" a synonym for "--list"
>  (this branch uses jk/branch-l-0-deprecation and jk/branch-l-1-removal.)

These three branches are interesting as all the work is done already.
We just have to wait? Anyway the first step looks good to me.

> * sb/blame-color (2018-02-13) 3 commits
>  - builtin/blame: highlight recently changed lines
>  - builtin/blame: add option to color metadata fields separately
>  - builtin/blame: dim uninteresting metadata
>
>  Expecting a reroll.
>  cf. https://public-inbox.org/git/20171110011002.10179-1-sbel...@google.com/#t
>  error messages are funny, can segfault, ...

I look into that again, I thought I had fixed the segfaults last time.

> * sb/object-store (2018-03-26) 27 commits
...
>  (this branch is used by sb/packfiles-in-repository; uses 
> nd/remove-ignore-env-field.)
>
>  Refactoring the internal global data structure to make it possible
>  to open multiple repositories, work with and then close them.
>
>  Rerolled by Duy on top of a separate preliminary clean-up topic.
>  The resulting structure of the topics looked very sensible.
>
>  Waiting for a follow-up discussion.

I had the impression the discussion had settled and this
would be ready for next.
https://public-inbox.org/git/xmqqy3iebpsw@gitster-ct.c.googlers.com/

> * sb/packfiles-in-repository (2018-03-26) 12 commits
...
>
>  (this branch uses nd/remove-ignore-env-field and sb/object-store.)
>
>  Refactoring of the internal global data structure continues.
>
>  Waiting for a follow-up discussion.

I'll review this one more closely and start a discussion.

> * bw/protocol-v2 (2018-03-15) 35 commits
>
>  The beginning of the next-gen transfer protocol.
>
>  Is everybody happy with this version?  One design decision with
>  larger consequence "to or not to build in?" has been settled in
>  favor of status quo, IIRC.

I skimmed over it and think it is ok to go.

> * en/rename-directory-detection (2018-02-27) 29 commits
>   (merged to 'next' on 2018-03-06 at d42470f86e)
...
>  Rename detection logic in "diff" family that is used in "merge" has
>  learned to guess when all of x/a, x/b and x/c have moved to z/a,
>  z/b and z/c, it is likely that x/d added in the meantime would also
>  want to move to z/d by taking the hint that the entire directory
>  'x' moved to 'z'.  A bug causing dirty files involved in a rename
>  to be overwritten during merge has also been fixed as part of this
>  work.
>
>  Will cook in 'next'.

I am excited about this one, despite having an issue this week
unrelated to this series, but related to rename detection.

The issue was an apparent rename from a tiny .c to a .java
file (also tiny), the license header was the large common part.

I wonder if we want get away from counting the lines that are
equal and instead want to go for some entropy based approach.
(When the license shows up in many files, they will become boring)

Another approach would be to give a "template" file to Git that knows
about which lines to exclude for the purpose of rename detection
(e.g. license, blank lines, indented braces, or such)

Thanks,
Stefan

Re: Apparent bug in credential tool running...

[Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)]

Sure, I can submit a patch if the change looks good to you (with my lack of 
experience in the git source and very rusty C I would, of course, defer to an 
expert in the area on exactly where to place the SIGPIPE ignore push and pop 
and such... but what's below seems to avoid the race for us so I can submit 
that as-is).

Thanks for the quick response!
Erik

On 3/28/18, 11:46 AM, "Jeff King"  wrote:

On Wed, Mar 28, 2018 at 06:26:08PM +, Erik Brady -X (brady - ROBERT 
HALF INTERNATIONAL INC at Cisco) wrote:

> The location of the problem is in credential.c, 
run_credential_helper()... this code:
> 
>...
> fp = xfdopen(helper.in, "w");
> credential_write(c, fp);
> fclose(fp);
>..
> 
> Which I think needs to become something like this:
> 
> fp = xfdopen(helper.in, "w");
> sigchain_push(SIGPIPE, SIG_IGN);
> credential_write(c, fp);
> fclose(fp);
> sigchain_pop(SIGPIPE);
> 
> The basics are that we wrote a credential helper in Go and, for the
> store action, it simply exits 0.  It is fast.  This is similar to the
> example here:

Yeah, that makes sense. Generally a pipe buffer would be plenty to hold
a credential, but we're racing against whether the other process exits
before we even write anything, so it's bound to fail eventually in a
racy way.

I don't think we've ever made a promise[1] about whether credential
helpers have to read their input, but it makes sense to me for Git to be
friendly and handle this case. We've done similar things for hooks.

Curiously, I have a very similar helper myself, which I did as an inline
shell snippet in my ~/.gitconfig:

  [credential "https://github.com;]
  username = peff
  helper = "!f() { test $1 = get && echo password=`pass peff/github/oauth`; 
}; f"

I guess I've never lost the race because of the sheer number of
sub-processes that get spawned (shell to "pass" which is itself a shell
script, which spawns gpg -- yikes!).

Do you want to send your change as a patch? There's some guidance in
Documentation/SubmittingPatches.

-Peff

[1] I know you pulled a similar example from the Pro Git book content,
which we mirror on git-scm.com.  The quality there is usually quite
good, but I don't consider it as authoritative as the manpages. :)

[RFC/PATCH] upload-pack: disable object filtering when disabled by config

[Jonathan Nieder]

When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,
2017-12-08), it was guarded by the uploadpack.allowFilter config item
to allow server operators to control when they start supporting it.

That config item didn't go far enough, though: it controls whether the
'filter' capability is advertised, but if a (custom) client ignores
the capability advertisement and passes a filter specification anyway,
the server would handle that despite allowFilter being false.

This is particularly significant if a security bug is discovered in
this new experimental partial clone code.  Installations without
uploadpack.allowFilter ought not to be affected since they don't
intend to support partial clone, but they would be swept up into being
vulnerable.

Simplify and limit the attack surface by making uploadpack.allowFilter
disable the feature, not just the advertisement of it.

NEEDSWORK: tests

Signed-off-by: Jonathan Nieder 
---
Noticed while reviewing the corresponding JGit code.

If this change seems like a good idea, I can add tests and re-send it
for real.

Thanks,
Jonathan

 Documentation/config.txt | 2 +-
 upload-pack.c| 8 
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ce9102cea8..4e0cff87f6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3364,7 +3364,7 @@ uploadpack.packObjectsHook::
stdout.
 
 uploadpack.allowFilter::
-   If this option is set, `upload-pack` will advertise partial
+   If this option is set, `upload-pack` will support partial
clone and partial fetch object filtering.
 +
 Note that this configuration variable is ignored if it is seen in the
diff --git a/upload-pack.c b/upload-pack.c
index f51b6cfca9..4a82602be5 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -69,7 +69,7 @@ static int stateless_rpc;
 static const char *pack_objects_hook;
 
 static int filter_capability_requested;
-static int filter_advertise;
+static int allow_filter;
 static struct list_objects_filter_options filter_options;
 
 static void reset_timeout(void)
@@ -846,7 +846,7 @@ static void receive_needs(void)
no_progress = 1;
if (parse_feature_request(features, "include-tag"))
use_include_tag = 1;
-   if (parse_feature_request(features, "filter"))
+   if (allow_filter && parse_feature_request(features, "filter"))
filter_capability_requested = 1;
 
o = parse_object(_buf);
@@ -976,7 +976,7 @@ static int send_ref(const char *refname, const struct 
object_id *oid,
 " allow-reachable-sha1-in-want" : "",
 stateless_rpc ? " no-done" : "",
 symref_info.buf,
-filter_advertise ? " filter" : "",
+allow_filter ? " filter" : "",
 git_user_agent_sanitized());
strbuf_release(_info);
} else {
@@ -1056,7 +1056,7 @@ static int upload_pack_config(const char *var, const char 
*value, void *unused)
if (!strcmp("uploadpack.packobjectshook", var))
return git_config_string(_objects_hook, var, 
value);
} else if (!strcmp("uploadpack.allowfilter", var)) {
-   filter_advertise = git_config_bool(var, value);
+   allow_filter = git_config_bool(var, value);
}
return parse_hide_refs_config(var, value, "uploadpack");
 }
-- 
2.17.0.rc1.321.gba9d0f2565

Re: What's cooking in git.git (Mar 2018, #05; Wed, 28)

[Brandon Williams]

On 03/28, Junio C Hamano wrote:
> * bw/protocol-v2 (2018-03-15) 35 commits
>  - remote-curl: don't request v2 when pushing
>  - remote-curl: implement stateless-connect command
>  - http: eliminate "# service" line when using protocol v2
>  - http: don't always add Git-Protocol header
>  - http: allow providing extra headers for http requests
>  - remote-curl: store the protocol version the server responded with
>  - remote-curl: create copy of the service name
>  - pkt-line: add packet_buf_write_len function
>  - transport-helper: introduce stateless-connect
>  - transport-helper: refactor process_connect_service
>  - transport-helper: remove name parameter
>  - connect: don't request v2 when pushing
>  - connect: refactor git_connect to only get the protocol version once
>  - fetch-pack: support shallow requests
>  - fetch-pack: perform a fetch using v2
>  - upload-pack: introduce fetch server command
>  - push: pass ref prefixes when pushing
>  - fetch: pass ref prefixes when fetching
>  - ls-remote: pass ref prefixes when requesting a remote's refs
>  - transport: convert transport_get_remote_refs to take a list of ref prefixes
>  - transport: convert get_refs_list to take a list of ref prefixes
>  - connect: request remote refs using v2
>  - ls-refs: introduce ls-refs server command
>  - serve: introduce git-serve
>  - test-pkt-line: introduce a packet-line test helper
>  - protocol: introduce enum protocol_version value protocol_v2
>  - transport: store protocol version
>  - connect: discover protocol version outside of get_remote_heads
>  - connect: convert get_remote_heads to use struct packet_reader
>  - transport: use get_refs_via_connect to get refs
>  - upload-pack: factor out processing lines
>  - upload-pack: convert to a builtin
>  - pkt-line: add delim packet support
>  - pkt-line: allow peeking a packet line without consuming it
>  - pkt-line: introduce packet_read_with_status
> 
>  The beginning of the next-gen transfer protocol.
> 
>  Is everybody happy with this version?  One design decision with
>  larger consequence "to or not to build in?" has been settled in
>  favor of status quo, IIRC.

I haven't heard anymore complaints with this version so I *think* it
should be good.

-- 
Brandon Williams

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

[Eric Sunshine]

On Wed, Mar 28, 2018 at 4:08 PM, Stefan Beller  wrote:
> On Wed, Mar 28, 2018 at 11:57 AM, Eric Sunshine  
> wrote:
>>> +test_expect_success 'moving the submodule does not break the superproject' 
>>> '
>>> +   (
>>> +   cd addtest2 &&
>>> +
>>> +   mv repo repo.bak &&
>>> +   git submodule status >actual &&
>>> +   grep -e "^-" -e repo actual &&
>>> +
>>> +   mv repo.bak repo
>>
>> Should this "move back" be encapsulated in a test_when_finished?
>
> I thought about that, but decided against it for some reason as I was debating
> where to put the test_when_finished. I mostly saw those at the very beginning
> of a test and wondered if it can be called from within a subshell.
> (I'd not want to put it at the beginning but rather adjacent to the move.)

It looks like test_when_finished() shouldn't be used in a subshell.
However, wouldn't the following be reasonable?

mv addtest2/repo addtest2/repo.bak &&
test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
(
cd addtest2 &&
git submodule status >actual &&
grep -e "^-" -e repo actual
)

Or, even simpler:

mv addtest2/repo addtest2/repo.bak &&
test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
git -C addtest2 submodule status >actual &&
grep -e "^-" -e repo actual

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

[Stefan Beller]

On Wed, Mar 28, 2018 at 11:57 AM, Eric Sunshine  wrote:
> On Wed, Mar 28, 2018 at 2:38 PM, Stefan Beller  wrote:
>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>> @@ -821,6 +821,18 @@ test_expect_success 'moving the superproject does not 
>> break submodules' '
>> +test_expect_success 'moving the submodule does not break the superproject' '
>> +   (
>> +   cd addtest2 &&
>> +
>> +   mv repo repo.bak &&
>> +   git submodule status >actual &&
>> +   grep -e "^-" -e repo actual &&
>> +
>> +   mv repo.bak repo
>
> Should this "move back" be encapsulated in a test_when_finished?

I thought about that, but decided against it for some reason as I was debating
where to put the test_when_finished. I mostly saw those at the very beginning
of a test and wondered if it can be called from within a subshell.
(I'd not want to put it at the beginning but rather adjacent to the move.)

What's cooking in git.git (Mar 2018, #05; Wed, 28)

[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.17 final is expected to be tagged by early next week.

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"]

* bp/refresh-cache-ent-rehash-fix (2018-03-15) 1 commit
  (merged to 'next' on 2018-03-15 at bac8745f08)
 + Fix bugs preventing adding updated cache entries to the name hash

 The codepath to replace an existing entry in the index had a bug in
 updating the name hash structure, which has been fixed.


* dp/merge-strategy-doc-fix (2018-03-19) 1 commit
  (merged to 'next' on 2018-03-20 at 317e077588)
 + Documentation/merge-strategies: typofix

 Doc fix.


* jh/fsck-promisors (2018-03-13) 1 commit
  (merged to 'next' on 2018-03-15 at 0c283dbe5e)
 + sha1_file: restore OBJECT_INFO_QUICK functionality

 A hotfix to a topic that graduated recently.


* jk/attributes-path-doc (2018-03-20) 1 commit
  (merged to 'next' on 2018-03-20 at e965f0c68c)
 + doc/gitattributes: mention non-recursive behavior

 Doc update.


* js/ming-strftime (2018-03-19) 1 commit
  (merged to 'next' on 2018-03-20 at a9ca8172c7)
 + mingw: abort on invalid strftime formats


* jt/transfer-fsck-with-promissor (2018-03-15) 2 commits
  (merged to 'next' on 2018-03-15 at 6d1ccc965b)
 + fetch-pack: do not check links for partial fetch
 + index-pack: support checking objects but not links

 The transfer.fsckobjects configuration tells "git fetch" to
 validate the data and connected-ness of objects in the received
 pack; the code to perform this check has been taught about the
 narrow clone's convention that missing objects that are reachable
 from objects in a pack that came from a promissor remote is OK.


* ks/t3200-typofix (2018-03-15) 1 commit
  (merged to 'next' on 2018-03-15 at 8b8d397787)
 + t/t3200: fix a typo in a test description

 Test typofix.


* ma/config-page-only-in-list-mode (2018-02-21) 3 commits
  (merged to 'next' on 2018-03-15 at 652430af12)
 + config: change default of `pager.config` to "on"
 + config: respect `pager.config` in list/get-mode only
 + t7006: add tests for how git config paginates

 In a way similar to how "git tag" learned to honor the pager
 setting only in the list mode, "git config" learned to ignore the
 pager setting when it is used for setting values (i.e. when the
 purpose of the operation is not to "show").


* ma/skip-writing-unchanged-index (2018-03-01) 1 commit
  (merged to 'next' on 2018-03-15 at cdbbc66464)
 + write_locked_index(): add flag to avoid writing unchanged index

 Internal API clean-up to allow write_locked_index() optionally skip
 writing the in-core index when it is not modified.


* ml/filter-branch-portability-fix (2018-03-19) 1 commit
  (merged to 'next' on 2018-03-20 at c7c17cfc8b)
 + filter-branch: use printf instead of echo -e

 Shell script portability fix.


* nd/parseopt-completion (2018-03-23) 2 commits
  (merged to 'next' on 2018-03-23 at 2bee77135e)
 + t9902: disable test on the list of merge-strategies under GETTEXT_POISON
  (merged to 'next' on 2018-03-22 at 279765c437)
 + completion: clear cached --options when sourcing the completion script
 (this branch is used by nd/parseopt-completion-more.)

 Hotfix for recently graduated topic that give help to completion
 scripts from the Git subcommands that are being completed


* pc/submodule-helper (2018-03-27) 1 commit
  (merged to 'next' on 2018-03-27 at 362e0ef09b)
 + submodule deinit: handle non existing pathspecs gracefully

 Hotfix.


* rj/http-code-cleanup (2018-03-15) 1 commit
  (merged to 'next' on 2018-03-15 at 0dfd462ff8)
 + http: fix an unused variable warning for 'curl_no_proxy'

 There was an unused file-scope static variable left in http.c when
 building for versions of libCURL that is older than 7.19.4, which
 has been fixed.

 This will become unnecessary, when we follow-through the
 jk/drop-ancient-curl topic.


* rj/warning-uninitialized-fix (2018-03-20) 2 commits
  (merged to 'next' on 2018-03-20 at 9ac9d02b0b)
 + read-cache: fix an -Wmaybe-uninitialized warning
 + -Wuninitialized: remove some 'init-self' workarounds

 Compilation fix.


* tg/stash-doc-typofix (2018-03-27) 1 commit
  (merged to 'next' on 2018-03-27 at 144a25f09c)
 + git-stash.txt: remove extra square bracket

 Hotfix.


* tz/complete-tag-delete-tagname (2018-03-19) 1 commit
  (merged to 'next' on 2018-03-20 at d63d45ff16)
 + completion: complete tags with git tag --delete/--verify


* tz/relnotes-1.7-on-perl (2018-03-16) 1 commit
  (merged to 'next' on 2018-03-20 at ed4b26e581)
 + RelNotes: add details on Perl module changes

--
[New 

[ANNOUNCE] Git v2.17.0-rc2

[Junio C Hamano]

A release candidate Git v2.17.0-rc2 is now available for testing
at the usual places.  It is comprised of 499 non-merge commits
since v2.16.0, contributed by 62 people, 19 of which are new faces.

I am hoping that we can have the final version tagged at the end of
coming weekend, before I fly out to Tokyo.  I expect to be offline
most of the next week after the final is tagged.

The tarballs are found at:

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

The following public repositories all have a copy of the
'v2.17.0-rc2' tag and the 'master' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = https://github.com/gitster/git

New contributors whose contributions weren't in v2.16.0 are as follows.
Welcome to the Git development community!

  Adam Borowski, Alban Gruin, Andreas G. Schacker, Bernhard
  M. Wiedemann, Christian Ludwig, Gargi Sharma, Genki Sky,
  Gregory Herrero, Jon Simons, Juan F. Codagnone, Kim Gybels,
  Lucas Werkmeister, Mathias Rav, Michele Locati, Motoki Seki,
  Stefan Moch, Stephen R Guglielmo, Tatyana Krasnukha, and Thomas
  Levesque.

Returning contributors who helped this release are as follows.
Thanks for your continued support.

  Ævar Arnfjörð Bjarmason, Alexander Shopov, Alex Bennée, Ben
  Peart, Brandon Williams, brian m. carlson, Christian Couder,
  Daniel Knittl-Frank, David Pursehouse, Derrick Stolee, Elijah
  Newren, Eric Sunshine, Eric Wong, Jason Merrill, Jeff Hostetler,
  Jeff King, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
  Junio C Hamano, Kaartic Sivaraam, Mårten Kongstad, Martin
  Ågren, Matthieu Moy, Michael Haggerty, Nathan Payre, Nguyễn
  Thái Ngọc Duy, Nicolas Morey-Chaisemartin, Olga Telezhnaya,
  Patryk Obara, Phillip Wood, Prathamesh Chavan, Ramsay Jones,
  Randall S. Becker, Rasmus Villemoes, René Scharfe, Robert
  P. J. Day, Stefan Beller, SZEDER Gábor, Thomas Gummerer,
  Todd Zullinger, Torsten Bögershausen, and Yasushi SHOJI.



Git 2.17 Release Notes (draft)
==

Updates since v2.16
---

UI, Workflows & Features

 * "diff" family of commands learned "--find-object=" option
   to limit the findings to changes that involve the named object.

 * "git format-patch" learned to give 72-cols to diffstat, which is
   consistent with other line length limits the subcommand uses for
   its output meant for e-mails.

 * The log from "git daemon" can be redirected with a new option; one
   relevant use case is to send the log to standard error (instead of
   syslog) when running it from inetd.

 * "git rebase" learned to take "--allow-empty-message" option.

 * "git am" has learned the "--quit" option, in addition to the
   existing "--abort" option; having the pair mirrors a few other
   commands like "rebase" and "cherry-pick".

 * "git worktree add" learned to run the post-checkout hook, just like
   "git clone" runs it upon the initial checkout.

 * "git tag" learned an explicit "--edit" option that allows the
   message given via "-m" and "-F" to be further edited.

 * "git fetch --prune-tags" may be used as a handy short-hand for
   getting rid of stale tags that are locally held.

 * The new "--show-current-patch" option gives an end-user facing way
   to get the diff being applied when "git rebase" (and "git am")
   stops with a conflict.

 * "git add -p" used to offer "/" (look for a matching hunk) as a
   choice, even there was only one hunk, which has been corrected.
   Also the single-key help is now given only for keys that are
   enabled (e.g. help for '/' won't be shown when there is only one
   hunk).

 * Since Git 1.7.9, "git merge" defaulted to --no-ff (i.e. even when
   the side branch being merged is a descendant of the current commit,
   create a merge commit instead of fast-forwarding) when merging a
   tag object.  This was appropriate default for integrators who pull
   signed tags from their downstream contributors, but caused an
   unnecessary merges when used by downstream contributors who
   habitually "catch up" their topic branches with tagged releases
   from the upstream.  Update "git merge" to default to --no-ff only
   when merging a tag object that does *not* sit at its usual place in
   refs/tags/ hierarchy, and allow fast-forwarding otherwise, to
   mitigate the problem.

 * "git status" can spend a lot of cycles to compute the relation
   between the current branch and its upstream, which can now be
   disabled with "--no-ahead-behind" option.

 * "git diff" and friends learned funcname patterns for Go language
   source files.

 * "git send-email" learned "--reply-to=" option.

 * Funcname pattern used for C# now recognizes "async" keyword.

 * In a way similar to how "git tag" learned to honor the pager
   setting only in the list mode, "git config" learned to ignore the
   pager setting when it is used for setting values 

Re: git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec

[Peter Oberndorfer]

On 2018-03-28 00:56, Stefan Beller wrote:
> On Tue, Mar 27, 2018 at 12:55 PM Peter Oberndorfer 
> wrote:

Hi,

as expected your patch fixed the BUG output.
Thanks!

>> 2) Should "git submodule deinit" work on submodules that were removed by
> upstream already?
> 
> To answer the question "Is this a submodule that upstream removed
> (recently)?"
> we'd have to put in some effort, essentially checking if that was ever a
> submodule
> (and not a directory or file).
> 

Hmm, yeah looks a bit more complicated than I initially imagined
since submodules can have a name that's different from their path.
And after the rebase, the name <-> path mapping via .gitmodules is not 
available anymore.

Naively I think it could work the following way:
* Either iterate over all submodules in .git/modules/ and check their config
  has a worktree = "../../path" that resolves to the submodule path we want to 
remove.
* Or check the "gitlink:" path in submodule/.git if it points to our 
.git/modules/
Then if .git/config contains a [submodule "name"] entry
we should have a pretty good idea if this folder contains a stale submodule.

> When using "git pull --recurse-submodules" the submodule ought to be removed
> automatically.
> 
> When doing a fetch && merge manually, we may want to teach merge to remove
> a submodule that we have locally upon merge, too.
> 

Yeah that would be nice :-)
In my case I updated the repository via a rebase, so that would also have to be 
covered.

> I view the git-submodule command as a bare bones plumbing helper, that we'd
> want
> to deprecate eventually as all other higher level commands will know how to
> deal
> with submodules.
> 
> So I think we do not want to teach "git submodule deinit" to remove dormant
> repositories, that were submodules removed by upstream already.
> 

My gut feeling makes me expect the following:
* It would be nice if such stale submodules showed up in "git submodule status" 
or "git status"
  Now "git submodule" shows nothing related to this stale submodule
  Now "git status" shows  Untracked files: src/rt which is a bit confusing as 
the actual submodule is in src/rt/hoedown
  Now "Git gui" shows src/rt/hoedown as untracked git repository
* There should be an easy(and safe) way for the user to deinit such a submodule
  if if the automatic submodule updating during a merge/rebase was not enabled 
or somehow failed.
(Minus the problem of somebody having to actually do the work...)

>> ~/src/rust/rust$ git submodule status
> ...
>>   b87873eaceb75cf9342d5273f01ba2c020f61ca8 src/tools/lld ((null))
> 
>> -> strangely I get (null) for the current branch/commit in some
> submodules?
> 
> This sounds like (3). Looking into that.

Sorry, what do you mean by (3)?

Thanks,
Greetings Peter

Re: [PATCH 5/6] submodule: fixup nested submodules after moving the submodule

[Stefan Beller]

>> + if (repo_init(, sub_gitdir, sub_worktree))
>> + return;
>
> Note that in Duy's object-store series he made this function private
> (IIRC) so this will result in some clash of the two series.
>

Yes, that is the case.
I wonder if I'd rather revert to v1 where we only use the
submodule repo init, or if we revert b2f0eceecf (repository:
initialize the_repository in main(), 2018-03-03) partially to
have repo_init available.

I would think the approach with submodule init is a bit cleaner
though has some more lines of code, using just repo_init
seems easier, but we really have no use case for a separate
repo_init unless they are submodules. And here we ought to
check for the repo being a submodule.

Not yet sure which path to take.

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

[Eric Sunshine]

On Wed, Mar 28, 2018 at 2:38 PM, Stefan Beller  wrote:
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> @@ -821,6 +821,18 @@ test_expect_success 'moving the superproject does not 
> break submodules' '
> +test_expect_success 'moving the submodule does not break the superproject' '
> +   (
> +   cd addtest2 &&
> +
> +   mv repo repo.bak &&
> +   git submodule status >actual &&
> +   grep -e "^-" -e repo actual &&
> +
> +   mv repo.bak repo

Should this "move back" be encapsulated in a test_when_finished?

> +   )
> +'

Re: Apparent bug in credential tool running...

[Jeff King]

On Wed, Mar 28, 2018 at 06:26:08PM +, Erik Brady -X (brady - ROBERT HALF 
INTERNATIONAL INC at Cisco) wrote:

> The location of the problem is in credential.c, run_credential_helper()... 
> this code:
> 
>...
> fp = xfdopen(helper.in, "w");
> credential_write(c, fp);
> fclose(fp);
>..
> 
> Which I think needs to become something like this:
> 
> fp = xfdopen(helper.in, "w");
> sigchain_push(SIGPIPE, SIG_IGN);
> credential_write(c, fp);
> fclose(fp);
> sigchain_pop(SIGPIPE);
> 
> The basics are that we wrote a credential helper in Go and, for the
> store action, it simply exits 0.  It is fast.  This is similar to the
> example here:

Yeah, that makes sense. Generally a pipe buffer would be plenty to hold
a credential, but we're racing against whether the other process exits
before we even write anything, so it's bound to fail eventually in a
racy way.

I don't think we've ever made a promise[1] about whether credential
helpers have to read their input, but it makes sense to me for Git to be
friendly and handle this case. We've done similar things for hooks.

Curiously, I have a very similar helper myself, which I did as an inline
shell snippet in my ~/.gitconfig:

  [credential "https://github.com;]
  username = peff
  helper = "!f() { test $1 = get && echo password=`pass peff/github/oauth`; }; 
f"

I guess I've never lost the race because of the sheer number of
sub-processes that get spawned (shell to "pass" which is itself a shell
script, which spawns gpg -- yikes!).

Do you want to send your change as a patch? There's some guidance in
Documentation/SubmittingPatches.

-Peff

[1] I know you pulled a similar example from the Pro Git book content,
which we mirror on git-scm.com.  The quality there is usually quite
good, but I don't consider it as authoritative as the manpages. :)

Re: [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved

[Duy Nguyen]

On Wed, Mar 28, 2018 at 8:30 PM, Jeff King  wrote:
> On Wed, Mar 28, 2018 at 07:55:35PM +0200, Nguyễn Thái Ngọc Duy wrote:
>
>> From: Duy Nguyen 
>>
>> As noted in the previous patch, when $CWD is moved, we recognize the
>> problem with relative paths and update $GIT_WORK_TREE and $GIT_DIR
>> with new ones.
>>
>> We have plenty more environment variables that can contain paths
>> though. If they are read and cached before setup_work_tree() is
>> called, nobody will update them and they become bad paths.
>
> Hmm, yeah, I missed these. It would be interesting to know if there are
> easy-to-run test cases that show off these bugs, or if they're
> hypothetical. (Even if they are hypothetical, I'm not opposed to fixing
> them in the name of maintainability).

It's kinda hard to show off these. But the GIT_ALTERNATE_OBJ..
variable could be one example in favor of this fix. Before, we lazily
read the env var which is most likely after setup_work_tree() has run.
After a bunch of code reorganization and stuff, GIT_ALTERNATE_OBJ is
now read very early, which should be safe (why not?) but it actually
breaks. Alternate db is only queried as the last resort if I'm not
mistaken, so 90% of time you just hit an object in odb and never find
out that your GIT_ALTERNATE_OBJ... points to nowhere.

>> diff --git a/environment.c b/environment.c
>> index 39b3d906c8..f9dcc1b99e 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -128,6 +128,20 @@ const char * const local_repo_env[] = {
>>   NULL
>>  };
>>
>> +/* A subset of local_repo_env[] that contains path */
>> +const char * const local_repo_path_env[] = {
>> + ALTERNATE_DB_ENVIRONMENT,
>> + CONFIG_ENVIRONMENT,
>> + DB_ENVIRONMENT,
>> + GIT_COMMON_DIR_ENVIRONMENT,
>> + GIT_DIR_ENVIRONMENT,
>> + GIT_SHALLOW_FILE_ENVIRONMENT,
>> + GIT_WORK_TREE_ENVIRONMENT,
>> + GRAFT_ENVIRONMENT,
>> + INDEX_ENVIRONMENT,
>> + NULL
>> +};
>
> It might be nice to fold this list into local_repo_env automatically. I
> think you'd have to do it with a macro.

Aha! I did not like the split either and wanted to turn
local_repo_env[] to an array of struct so we can add attributes to
each variable, but the way local_repo_env[] is being used, that's
impossible without more surgery.

> OTOH, it's possible that there could be a path-related environment
> variable that _isn't_ actually part of local_repo_env. E.g., I think
> GIT_CONFIG might classify there (though I don't know if it's worth
> worrying about).

I'd rather fix it now and forget about it than trying to troubleshoot
a problem related to bad relative $GIT_CONFIG (even if the chance of
that happening is probably 1%)

>> +static void update_path_envs(const char *old_cwd, const char *new_cwd,
>> +  void *cb)
>> +{
>> + int i;
>> +
>> + /*
>> +  * FIXME: special treatment needed for
>> +  * GIT_ALTERNATE_OBJECT_DIRECTORIES because it can contain
>> +  * multiple paths.
>> +  */
>
> Yuck. It just keeps getting more complicated. :(
>
> I do wonder if relative paths in variables like this are worth worrying
> about. AFAIK, it's always been a "well, it kind of works" situation, but

Yeah. 99% of time $GIT_DIR is $GIT_WORK_TREE/.git and no path
adjustment is needed.

> not something we've tried to actively support. I think with the current
> code you'd potentially get inconsistent results between a command which
> sets up the work tree and one which doesn't. So this would be fixing
> that, but at the same time, I'm not sure how much we want to promise
> here.

I would be just as happy to die() when we find out we have relative
paths that takes too much work to reparent. It keeps the amount of
work down and will not bite us later (and will let us know if any user
needs it because they would have to report back after hitting the said
die()).
-- 
Duy

[PATCH] submodule: check for NULL return of get_submodule_ref_store()

[Stefan Beller]

From: René Scharfe 

If we can't find a ref store for a submodule then assume it the latter
is not initialized (or was removed).  Print a status line accordingly
instead of causing a segmentation fault by passing NULL as the first
parameter of refs_head_ref().

Reported-by: Jeremy Feusi 
Signed-off-by: Rene Scharfe 
Signed-off-by: Stefan Beller 
---

I added a test for you.

Thanks,
Stefan

 builtin/submodule--helper.c |  8 ++--
 t/t7400-submodule-basic.sh  | 12 
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ee020d4749..ae3014ac5a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -654,9 +654,13 @@ static void status_submodule(const char *path, const 
struct object_id *ce_oid,
 displaypath);
} else if (!(flags & OPT_CACHED)) {
struct object_id oid;
+   struct ref_store *refs = get_submodule_ref_store(path);
 
-   if (refs_head_ref(get_submodule_ref_store(path),
- handle_submodule_head_ref, ))
+   if (!refs) {
+   print_status(flags, '-', path, ce_oid, displaypath);
+   goto cleanup;
+   }
+   if (refs_head_ref(refs, handle_submodule_head_ref, ))
die(_("could not resolve HEAD ref inside the "
  "submodule '%s'"), path);
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index a39e69a3eb..d8aee51603 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -821,6 +821,18 @@ test_expect_success 'moving the superproject does not 
break submodules' '
)
 '
 
+test_expect_success 'moving the submodule does not break the superproject' '
+   (
+   cd addtest2 &&
+
+   mv repo repo.bak &&
+   git submodule status >actual &&
+   grep -e "^-" -e repo actual &&
+
+   mv repo.bak repo
+   )
+'
+
 test_expect_success 'submodule add --name allows to replace a submodule with 
another at the same path' '
(
cd addtest2 &&
-- 
2.17.0.rc1.321.gba9d0f2565-goog

Apparent bug in credential tool running...

[Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)]

Hi git Experts,

I believe we've encountered a bug in git.  I built the latest, git 2.16.3, and 
it still appears to be a problem.  I'm not a git expert and my C is 
ridiculously rusty but I think a co-worker and I figured it out... apologies if 
we are incorrect in any assumptions (as I do not wish to waste anyone's time).

The location of the problem is in credential.c, run_credential_helper()... this 
code:

   ...
fp = xfdopen(helper.in, "w");
credential_write(c, fp);
fclose(fp);
   ..

Which I think needs to become something like this:

fp = xfdopen(helper.in, "w");
sigchain_push(SIGPIPE, SIG_IGN);
credential_write(c, fp);
fclose(fp);
sigchain_pop(SIGPIPE);

The basics are that we wrote a credential helper in Go and, for the store 
action, it simply exits 0.  It is fast.  This is similar to the example here:

https://git-scm.com/book/en/v2/Git-Tools-Credential-Storage#_a_custom_credential_cache

Which is, of course, in Ruby, not Go (so, not so fast).  It exits if it is not 
a get cred helper action without reading stdin.  Anyhow, for our credential 
helper the store operation completes very quickly.  What we've found is that 
occasionally the git command will be killed off just after running the 
credential store operation.  We can see that our credential helper is being 
fired up (it has a debug mode) and it quickly exits.  We can see that git dies 
on the fclose(fp) by putting trace_printf() calls before and after that fclose 
in the git source code (ie: the trace message before the fclose() prints, the 
trace message after the fclose does not).

Our belief is that the write is buffered and written at the time of fclose()... 
and that the credential helper tool has already exited "sometimes" (as it is 
very fast, but even so this doesn't fail very often).  For those times when our 
cred helper has exited "quickly enough", a SIGPIPE can be generated... and, as 
SIGPIPE is not ignored, git goes "kaboom!" and dies.

To catch this scenario we wrote a simple hack Perl tool to run a bunch of 
serial git ls-remote commands like so:

#!/usr/cisco/bin/perl -w

$ENV{'GIT_TRACE'} = 2;
$ENV{'GIT_TRACE_CURL'} = 2;
$ENV{'GIT_TRACE_PERFORMANCE'} = 1;
$ENV{'GIT_TRACE_PACKET'} = 1;

for ( my $i = 1; $i<=10; $i++) {
print("Run: $i\n");
my $output = `/ws/brady-sjc/git/git-2.16.3/bin/git ls-remote 
https://hostname.company.com/git/path/repo.git HEAD 2>&1`;
if ( $? != 0 ) {
print("FAIL: output:\n$output\n");
exit(1);
}
}
exit(0);

The problem seemed to come up most commonly on older linux VM's... in this case 
running 2.6.18-416.el5 #1 SMP.  The tool will iterate for a while and then, 
boom, it blows up (usually within 1000 iterations, sometimes a couple/few 
thousand but usually well before that).

Anyhow... we did a few things to test our theory that it is, indeed, SIGPIPE 
causing git to exit:

1) My co-worker modified our credential manager to read in the data sent by git 
before exiting... that solved the problem as we accept the data written so the 
child is still there and no SIGPIPE is generated... this is our current 
workaround (so we are OK, but would be good to fix this in the git code we 
think)

2) I modified the above code to use a signal handler in credential.c (instead 
of SIG_IGN) and put a trace_printf() and an exit(1) inside the signal 
handler... similar to the problem we're seeing it'll run a bunch successfully 
until, boom, timing is hit such that the child exits quickly enough and causes 
the SIGPIPE to occur at which point git is killed so using the cheesy Perl 
test script it ran through a couple hundred iterations fine and then a SIGPIPE 
was seen and it died in the signal handler I wrote... so clearly SIGPIPE is 
being generated but only occasionally (it is timing based, so occurs only now 
and then... and we almost never see it on some of our boxes)

3) I modified the git code (using our old cred helper which exits quickly) per 
the above recommended credential.c changes (you folks can likely do a better 
fix)... and re-run the Perl test script... no longer fails now that we are 
ignoring SIGPIPE (ran about 20,000+ iterations).

Note that the build-in credential manager was not failing... it reads the cred 
helper store data so it would not have the problem.

Let me know if you need any additional information... and thanks for your time 
and consideration.

Erik
br...@cisco.com

Good-Day!

[sar...@ono.com]

--
Good-Day!
 
I am Malik Sanfo,I' m from Burkina Faso, I found your LinkedIn profile 
suit for a business proposition which sufficient benefits. I shall 
elaborate on this project upon your swift reply.
 
Sincerely
Mr. Malik Sanfo
--

Re: [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved

[Jeff King]

On Wed, Mar 28, 2018 at 07:55:35PM +0200, Nguyễn Thái Ngọc Duy wrote:

> From: Duy Nguyen 
> 
> As noted in the previous patch, when $CWD is moved, we recognize the
> problem with relative paths and update $GIT_WORK_TREE and $GIT_DIR
> with new ones.
> 
> We have plenty more environment variables that can contain paths
> though. If they are read and cached before setup_work_tree() is
> called, nobody will update them and they become bad paths.

Hmm, yeah, I missed these. It would be interesting to know if there are
easy-to-run test cases that show off these bugs, or if they're
hypothetical. (Even if they are hypothetical, I'm not opposed to fixing
them in the name of maintainability).

> Hook into setup_work_tree() and update all those env variables. The
> code to update $GIT_WORK_TREE is no longer needed and removed.

That's a nice cleanup.

> Technically we should remove the setenv() in set_git_dir() as well,
> but that is also called _not_ by setup_work_tree() and we should keep
> the behavior the same in that case for safety. set_git_dir() will be
> removed from setup_work_tree() soon, which achieves the same goal.

Makes sense.

> diff --git a/environment.c b/environment.c
> index 39b3d906c8..f9dcc1b99e 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -128,6 +128,20 @@ const char * const local_repo_env[] = {
>   NULL
>  };
>  
> +/* A subset of local_repo_env[] that contains path */
> +const char * const local_repo_path_env[] = {
> + ALTERNATE_DB_ENVIRONMENT,
> + CONFIG_ENVIRONMENT,
> + DB_ENVIRONMENT,
> + GIT_COMMON_DIR_ENVIRONMENT,
> + GIT_DIR_ENVIRONMENT,
> + GIT_SHALLOW_FILE_ENVIRONMENT,
> + GIT_WORK_TREE_ENVIRONMENT,
> + GRAFT_ENVIRONMENT,
> + INDEX_ENVIRONMENT,
> + NULL
> +};

It might be nice to fold this list into local_repo_env automatically. I
think you'd have to do it with a macro.

OTOH, it's possible that there could be a path-related environment
variable that _isn't_ actually part of local_repo_env. E.g., I think
GIT_CONFIG might classify there (though I don't know if it's worth
worrying about).

> +static void update_path_envs(const char *old_cwd, const char *new_cwd,
> +  void *cb)
> +{
> + int i;
> +
> + /*
> +  * FIXME: special treatment needed for
> +  * GIT_ALTERNATE_OBJECT_DIRECTORIES because it can contain
> +  * multiple paths.
> +  */

Yuck. It just keeps getting more complicated. :(

I do wonder if relative paths in variables like this are worth worrying
about. AFAIK, it's always been a "well, it kind of works" situation, but
not something we've tried to actively support. I think with the current
code you'd potentially get inconsistent results between a command which
sets up the work tree and one which doesn't. So this would be fixing
that, but at the same time, I'm not sure how much we want to promise
here.

-Peff

Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King]

On Wed, Mar 28, 2018 at 07:55:29PM +0200, Nguyễn Thái Ngọc Duy wrote:

> >> The problem is switching relative paths relies on the old $CWD if I'm
> >> not mistaken and we need  getcwd() for this. I'd love to have one
> >> callback that says "$CWD has been switched from this path to that
> >> path, do whatever you need to" that can be called any time, before or
> >> after chdir(). I'll look more into it.
> >
> > I think it should be OK to save getcwd() and just construct the original
> > path after the fact. Here's some patches which do that in a nice way.
> 
> Heh.. I should have checked mails more often while coding ;-)

I was worried about that. I started this earlier as a "well, I could
probably just knock this out quickly" task. Many hours later, I found
there were quite a few subtleties. :)

> This is what I got, which is slightly different from your series
> because I want to call set_git_dir() just one time (by
> setup_git_directory) and never again. I think the API looks close
> enough.

Yeah, I actually think after this series we could enforce that
set_git_dir() is never called twice.

I think we could even make sure that repo_set_gitdir() is never called
twice, too, but it would involve a little more surgery (we'd have to
teach it to set up a child_notify handler, and the way it interacts with
the set_git_dir() one is subtle. I did try it and it worked, but I went
with what you saw in patch 3 because it was simpler).

> I will probably rework on top of your chdir-notify instead (and let
> yours to be merged earlier)

Thanks. I like some of the related changes you made, like including this
in the tracing output. That should be easy to do on top of mine, I
think.

I didn't look for other spots that might need similar treatment (like
the object-store bits).

-Peff

Re: [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix)

[Duy Nguyen]

On Wed, Mar 28, 2018 at 8:02 PM, Stefan Beller  wrote:
> On Wed, Mar 28, 2018 at 10:55 AM, Nguyễn Thái Ngọc Duy
>  wrote:
>> +/**
>> + * Return the current directory, fall back to $PWD if the
>> + * current directory does not exist.
>> + */
>> +extern void strbuf_get_pwd_cwd(struct strbuf *sb);
>
> Please see 89a9f2c862 (CodingGuidelines: mention "static" and "extern",
> 2018-02-08) and drop the extern if you need to reroll.

I'm aware (and in favor) of that actually. But this whole strbuf.h
uses extern. Either I had to delete all extern first, or go with old
style and maybe do the whole deletion later. I chose the former. Maybe
be I'll delete all "extern" as a prep patch.
-- 
Duy

Re: [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix)

[Stefan Beller]

On Wed, Mar 28, 2018 at 10:55 AM, Nguyễn Thái Ngọc Duy
 wrote:
> +/**
> + * Return the current directory, fall back to $PWD if the
> + * current directory does not exist.
> + */
> +extern void strbuf_get_pwd_cwd(struct strbuf *sb);

Please see 89a9f2c862 (CodingGuidelines: mention "static" and "extern",
2018-02-08) and drop the extern if you need to reroll.

Thanks,
Stefan

Re: [PATCH 2/4] add chdir-notify API

[Jeff King]

On Wed, Mar 28, 2018 at 01:58:18PM -0400, Eric Sunshine wrote:

> On Wed, Mar 28, 2018 at 1:40 PM, Jeff King  wrote:
> > [...]
> > Let's provide an API to let code that stores relative paths
> > "subscribe" to updates to the current working directory.
> > This means that callers of chdir() don't need to know about
> > all subscribers ahead of time; they can simply consult a
> > dynamically built list.
> > [...]
> > Signed-off-by: Jeff King 
> > ---
> > diff --git a/chdir-notify.c b/chdir-notify.c
> > @@ -0,0 +1,71 @@
> > +int chdir_notify(const char *new_cwd)
> > +{
> > +   struct strbuf old_cwd = STRBUF_INIT;
> > +   struct list_head *pos;
> > +
> > +   if (strbuf_getcwd(_cwd) < 0)
> > +   return -1;
> > +   if (chdir(new_cwd) < 0)
> > +   return -1;
> 
> This 'return' is leaking 'old_cwd', isn't it?

Whoops, yes. I wrote it the other way around first to avoid the leak,
but of course that has other problems. ;)

> > diff --git a/chdir-notify.h b/chdir-notify.h
> > @@ -0,0 +1,64 @@
> > + * In practice most callers will want to move a relative path to the new 
> > root;
> > + * they can use the reparent_relative_path() helper for that. If that's all
> > + * you're doing, you can also use the convenience function:
> > + *
> > + *   chdir_notify_reparent(_path);
> > + */
> > +typedef void (*chdir_notify_callback)(const char *old_cwd,
> > + const char *new_cwd,
> > + void *data);
> > +void chdir_notify_register(chdir_notify_callback cb, void *data);
> > +void chdir_notify_reparent(char **path);
> 
> Can we have some documentation here (or in the chdir_notify_reparent()
> example above) explaining that *path is a heap-allocated value? I had
> to consult the implementation to understand ownership.

Sure. I originally had reparent_relative_path() handle the freeing and
allocation, and it explained the ownership in its docstring. But as I
revised that to simply return its value, that got lost. I'll add it back
in here.

-Peff

Re: [PATCH] t1300: document some aesthetic failures of the config editor

[Jeff King]

On Wed, Mar 28, 2018 at 06:33:55PM +0200, Johannes Schindelin wrote:

> On Fri, 29 Mar 2013, Jeff King wrote:
> 
> > Subject: [PATCH] t1300: document some aesthetic failures of the config 
> > editor

This is an old one. :) I had to go look up the old thread to refresh
myself.

> [...]
> Obviously, your example gives the impression that `git config --unset
> core.key` shoud *delete* that comment (that obviously is intended to
> document the section, not the `key` value).
> 
> And this is bad, really bad. And this comment does not make it better:
> 
>   I think we may not attain that ideal without some natural language
>   processing of the comments. But hey, no reason not to shoot for the
>   stars. :)
> 
> There *is* a reason, a very good reason *not* to shoot for the stars.

I think you are reading more into my comment than was intended. No, I
don't think we plan to implement a sufficiently advanced AI to cover all
these cases. But as I said in the thread:

  It makes sense to me to document both via tests; even if we end up
  tweaking the expected behavior when the fix is actually implemented,
  the presence of the test still serves as a reminder of the issue.

So it was always intended for this test to give a general sense of the
problem, from which somebody interested could dig further and work on
it.

Probably the commit message could have made this more clear (or even an
in-code comment).

> Think about it. The `test_expect_failure` function is intended to
> demonstrate bugs, and once those bugs are fixed, the _failure should be
> turned into _success. And if somebody looks for work, they can look for
> test_expect_failure and find tons of micro-projects.
> 
> What you did there was to change some valid demonstration of a bug that
> can be fixed to something that cannot be fixed. So if an occasional lurker
> comes along, sees what you expect to be fixed, they would have said
> "Whoa!" and you lost a contribution.

Hypothetically, you may be right. But don't all bugs have some element
of this? People can find an expect_failure as a starting point, but
they'll have to dig into the background and history of the bug if they
want to know the subtleties. This one is just more subtle than some
others.

> On a positive note: I just finished work on a set of patches addressing
> this:
> https://github.com/git/git/compare/master...dscho:empty-config-section (I
> plan on submitting this tomorrow)

Great. If your series throws away my test and replaces it with something
more attainable (preferably with expect_success ;) ), I think that is
certainly a positive change.

-Peff

Re: [PATCH 2/4] add chdir-notify API

[Eric Sunshine]

On Wed, Mar 28, 2018 at 1:40 PM, Jeff King  wrote:
> [...]
> Let's provide an API to let code that stores relative paths
> "subscribe" to updates to the current working directory.
> This means that callers of chdir() don't need to know about
> all subscribers ahead of time; they can simply consult a
> dynamically built list.
> [...]
> Signed-off-by: Jeff King 
> ---
> diff --git a/chdir-notify.c b/chdir-notify.c
> @@ -0,0 +1,71 @@
> +int chdir_notify(const char *new_cwd)
> +{
> +   struct strbuf old_cwd = STRBUF_INIT;
> +   struct list_head *pos;
> +
> +   if (strbuf_getcwd(_cwd) < 0)
> +   return -1;
> +   if (chdir(new_cwd) < 0)
> +   return -1;

This 'return' is leaking 'old_cwd', isn't it?

> +   list_for_each(pos, _notify_entries) {
> +   struct chdir_notify_entry *e =
> +   list_entry(pos, struct chdir_notify_entry, list);
> +   e->cb(old_cwd.buf, new_cwd, e->data);
> +   }
> +
> +   strbuf_release(_cwd);
> +   return 0;
> +}
> diff --git a/chdir-notify.h b/chdir-notify.h
> @@ -0,0 +1,64 @@
> + * In practice most callers will want to move a relative path to the new 
> root;
> + * they can use the reparent_relative_path() helper for that. If that's all
> + * you're doing, you can also use the convenience function:
> + *
> + *   chdir_notify_reparent(_path);
> + */
> +typedef void (*chdir_notify_callback)(const char *old_cwd,
> + const char *new_cwd,
> + void *data);
> +void chdir_notify_register(chdir_notify_callback cb, void *data);
> +void chdir_notify_reparent(char **path);

Can we have some documentation here (or in the chdir_notify_reparent()
example above) explaining that *path is a heap-allocated value? I had
to consult the implementation to understand ownership.

[PATCH 7/8] repository: adjust repo paths when $CWD moves

[Nguyễn Thái Ngọc Duy]

From: Duy Nguyen 

As noted in previous and previous patches, when setup_work_tree()
moves $CWD, it calls set_git_dir() with a new path to make sure
relative $GIT_DIR is still correct.

The same problem from the previous patch applies here: if
$GIT_ALTERNATE_OBJECT_DIRECTORIES or $GIT_OBJECT_DIRECTORY are also
relative, the user is screwed.

Fortuntely the previous patch indirectly fixes this as well:
update_path_envs() in environment.c is actually called before
setup_git_env() is indirectly called by setup_work_tree() via
set_git_dir(). At that point, it correctly gets the updated paths from
env and pass them down. This patch is here for another reason.

>From the design perspective, calling set_git_dir() the second time [1]
is just bad because it could potentially be used to point repo code to
_another_ repository. The promise that "the new path actually points
to the same place as the old one" is not guaranteed and also hard to
see unless you know setup code well.

This patch avoids this. The second set_git_dir() call is just a
workaround to adjust paths. We can now do it directly from repo code
by subscribing to the "$CWD updated" event and do everything needed by
the repo. After this we could die() on subsequent repo_set_gitdir()
calls because we do not support it (we never did).

[1] The first time is done by setup_git_directory() code, which is
correct: we have just found a repo and we want to initialize our code
to read from that repo.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 object-store.h |  3 +++
 object.c   | 15 +++
 repository.c   | 28 
 setup.c|  6 +-
 4 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/object-store.h b/object-store.h
index fef33f345f..4af2c53bca 100644
--- a/object-store.h
+++ b/object-store.h
@@ -119,6 +119,9 @@ struct raw_object_store {
 };
 
 struct raw_object_store *raw_object_store_new(void);
+void raw_object_store_adjust_paths(struct raw_object_store *o,
+  const char *old_cwd,
+  const char *new_cwd);
 void raw_object_store_clear(struct raw_object_store *o);
 
 /*
diff --git a/object.c b/object.c
index a0a756f24f..5318922efd 100644
--- a/object.c
+++ b/object.c
@@ -457,6 +457,21 @@ struct raw_object_store *raw_object_store_new(void)
return o;
 }
 
+void raw_object_store_adjust_paths(struct raw_object_store *o,
+  const char *old_cwd,
+  const char *new_cwd)
+{
+   /*
+* "main repo" is technically wrong because we don't know that
+* from here. But it's the only repo that will execute this
+* function for now.
+*/
+   setup_adjust_path("main repo's object dir",
+ >objectdir, old_cwd, new_cwd);
+   setup_adjust_path("main repo's alt dir",
+ >alternate_db, old_cwd, new_cwd);
+}
+
 static void free_alt_odb(struct alternate_object_database *alt)
 {
strbuf_release(>scratch);
diff --git a/repository.c b/repository.c
index a4848c1bd0..c2edf227fe 100644
--- a/repository.c
+++ b/repository.c
@@ -8,6 +8,10 @@
 static struct repository the_repo;
 struct repository *the_repository;
 
+static void repo_adjust_paths(const char *old_cwd,
+ const char *new_cwd,
+ void *cb_data);
+
 void initialize_the_repository(void)
 {
the_repository = _repo;
@@ -15,6 +19,11 @@ void initialize_the_repository(void)
the_repo.index = _index;
the_repo.objects = raw_object_store_new();
repo_set_hash_algo(_repo, GIT_HASH_SHA1);
+   /*
+* non-main repos probably does not need this since $CWD should
+* never change again by the time they are created.
+*/
+   add_cwd_update_callback(repo_adjust_paths, the_repository);
 }
 
 static void expand_base_dir(char **out, const char *in,
@@ -70,6 +79,25 @@ void repo_set_gitdir(struct repository *repo,
repo->gitdir, "index");
 }
 
+static void repo_adjust_paths(const char *old_cwd, const char *new_cwd,
+ void *cb_data)
+{
+   struct repository *repo = cb_data;
+
+   if (repo != the_repository)
+   die("BUG: this is supposed to happen to main repo only");
+
+   setup_adjust_path("main repo's gitdir",
+ >gitdir, old_cwd, new_cwd);
+   setup_adjust_path("main repo's commondir",
+ >commondir, old_cwd, new_cwd);
+   setup_adjust_path("main repo's graft file",
+ >graft_file, old_cwd, new_cwd);
+   setup_adjust_path("main repo's index file",
+ >index_file, old_cwd, new_cwd);
+   raw_object_store_adjust_paths(repo->objects, old_cwd, new_cwd);
+}
+
 void repo_set_hash_algo(struct repository *repo, int hash_algo)
 {

[PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved

[Nguyễn Thái Ngọc Duy]

From: Duy Nguyen 

As noted in the previous patch, when $CWD is moved, we recognize the
problem with relative paths and update $GIT_WORK_TREE and $GIT_DIR
with new ones.

We have plenty more environment variables that can contain paths
though. If they are read and cached before setup_work_tree() is
called, nobody will update them and they become bad paths.

Hook into setup_work_tree() and update all those env variables. The
code to update $GIT_WORK_TREE is no longer needed and removed.

Technically we should remove the setenv() in set_git_dir() as well,
but that is also called _not_ by setup_work_tree() and we should keep
the behavior the same in that case for safety. set_git_dir() will be
removed from setup_work_tree() soon, which achieves the same goal.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 environment.c | 46 ++
 setup.c   |  7 ---
 2 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/environment.c b/environment.c
index 39b3d906c8..f9dcc1b99e 100644
--- a/environment.c
+++ b/environment.c
@@ -128,6 +128,20 @@ const char * const local_repo_env[] = {
NULL
 };
 
+/* A subset of local_repo_env[] that contains path */
+const char * const local_repo_path_env[] = {
+   ALTERNATE_DB_ENVIRONMENT,
+   CONFIG_ENVIRONMENT,
+   DB_ENVIRONMENT,
+   GIT_COMMON_DIR_ENVIRONMENT,
+   GIT_DIR_ENVIRONMENT,
+   GIT_SHALLOW_FILE_ENVIRONMENT,
+   GIT_WORK_TREE_ENVIRONMENT,
+   GRAFT_ENVIRONMENT,
+   INDEX_ENVIRONMENT,
+   NULL
+};
+
 static char *expand_namespace(const char *raw_namespace)
 {
struct strbuf buf = STRBUF_INIT;
@@ -149,6 +163,32 @@ static char *expand_namespace(const char *raw_namespace)
return strbuf_detach(, NULL);
 }
 
+static void update_path_envs(const char *old_cwd, const char *new_cwd,
+void *cb)
+{
+   int i;
+
+   /*
+* FIXME: special treatment needed for
+* GIT_ALTERNATE_OBJECT_DIRECTORIES because it can contain
+* multiple paths.
+*/
+   for (i = 0; local_repo_path_env[i]; i++) {
+   const char *name = local_repo_path_env[i];
+   const char *value = getenv(name);
+   char *new_value;
+
+   if (!value)
+   continue;
+   if (is_absolute_path(value))
+   continue;
+   new_value = xstrdup(value);
+   setup_adjust_path(name, _value, old_cwd, new_cwd);
+   if (setenv(name, new_value, 10))
+   error(_("could not set %s to '%s'"), name, value);
+   free(new_value);
+   }
+}
 /*
  * Wrapper of getenv() that returns a strdup value. This value is kept
  * in argv to be freed later.
@@ -170,6 +210,12 @@ void setup_git_env(const char *git_dir)
const char *replace_ref_base;
struct set_gitdir_args args = { NULL };
struct argv_array to_free = ARGV_ARRAY_INIT;
+   static int added_cwd_callback;
+
+   if (!added_cwd_callback) {
+   add_cwd_update_callback(update_path_envs, NULL);
+   added_cwd_callback = 1;
+   }
 
args.commondir = getenv_safe(_free, GIT_COMMON_DIR_ENVIRONMENT);
args.object_dir = getenv_safe(_free, DB_ENVIRONMENT);
diff --git a/setup.c b/setup.c
index e340ee2130..23b8f89ce2 100644
--- a/setup.c
+++ b/setup.c
@@ -435,13 +435,6 @@ void setup_work_tree(void)
if (!work_tree || chdir(work_tree))
die(_("this operation must be run in a work tree"));
 
-   /*
-* Make sure subsequent git processes find correct worktree
-* if $GIT_WORK_TREE is set relative
-*/
-   if (getenv(GIT_WORK_TREE_ENVIRONMENT))
-   setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
-
for (i = 0; i < nr_cwd_callbacks; i++)
cwd_callbacks[i].fn(old_cwd.buf, work_tree,
cwd_callbacks[i].cb_data);
-- 
2.17.0.rc1.439.gca064e2955

[PATCH 8/8] refs: adjust main repo paths when $CWD moves

[Nguyễn Thái Ngọc Duy]

From: Duy Nguyen 

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c| 10 ++
 refs/files-backend.c  | 15 +++
 refs/packed-backend.c | 12 
 refs/refs-internal.h  |  4 
 setup.c   |  1 +
 5 files changed, 42 insertions(+)

diff --git a/refs.c b/refs.c
index 8b7a77fe5e..2457a31d4d 100644
--- a/refs.c
+++ b/refs.c
@@ -1651,12 +1651,22 @@ static struct ref_store *ref_store_init(const char 
*gitdir,
return refs;
 }
 
+static void ref_store_adjust_paths(const char *old_cwd,
+  const char *new_cwd,
+  void *cb)
+{
+   struct ref_store *refs = cb;
+
+   refs->be->adjust_paths(refs, old_cwd, new_cwd);
+}
+
 struct ref_store *get_main_ref_store(void)
 {
if (main_ref_store)
return main_ref_store;
 
main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);
+   add_cwd_update_callback(ref_store_adjust_paths, main_ref_store);
return main_ref_store;
 }
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bec8e30e9e..d35a8db844 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3092,6 +3092,20 @@ static int files_reflog_expire(struct ref_store 
*ref_store,
return -1;
 }
 
+static void files_adjust_paths(struct ref_store *ref_store,
+  const char *old_cwd,
+  const char *new_cwd)
+{
+   struct files_ref_store *refs =
+   files_downcast(ref_store, REF_STORE_WRITE,
+  "files_adjust_paths");
+
+   setup_adjust_path("ref store's gitdir",
+ >gitdir, old_cwd, new_cwd);
+   setup_adjust_path("ref store's commondir",
+ >gitcommondir, old_cwd, new_cwd);
+}
+
 static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
 {
struct files_ref_store *refs =
@@ -3117,6 +3131,7 @@ struct ref_storage_be refs_be_files = {
"files",
files_ref_store_create,
files_init_db,
+   files_adjust_paths,
files_transaction_prepare,
files_transaction_finish,
files_transaction_abort,
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 65288c6472..764d1250a5 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1047,6 +1047,17 @@ int packed_refs_is_locked(struct ref_store *ref_store)
return is_lock_file_locked(>lock);
 }
 
+static void packed_adjust_paths(struct ref_store *ref_store,
+   const char *old_cwd,
+   const char *new_cwd)
+{
+   struct packed_ref_store *refs =
+   packed_downcast(ref_store, REF_STORE_WRITE,
+   "packed_adjust_paths");
+
+   setup_adjust_path("packed-refs", >path, old_cwd, new_cwd);
+}
+
 /*
  * The packed-refs header line that we write out. Perhaps other traits
  * will be added later.
@@ -1632,6 +1643,7 @@ struct ref_storage_be refs_be_packed = {
"packed",
packed_ref_store_create,
packed_init_db,
+   packed_adjust_paths,
packed_transaction_prepare,
packed_transaction_finish,
packed_transaction_abort,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index dd834314bd..73480543c0 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -505,6 +505,9 @@ struct ref_store;
  */
 typedef struct ref_store *ref_store_init_fn(const char *gitdir,
unsigned int flags);
+typedef void ref_store_adjust_paths_fn(struct ref_store *refs,
+  const char *old_cwd,
+  const char *new_cwd);
 
 typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);
 
@@ -625,6 +628,7 @@ struct ref_storage_be {
const char *name;
ref_store_init_fn *init;
ref_init_db_fn *init_db;
+   ref_store_adjust_paths_fn *adjust_paths;
 
ref_transaction_prepare_fn *transaction_prepare;
ref_transaction_finish_fn *transaction_finish;
diff --git a/setup.c b/setup.c
index e364aea7e5..35e89a03e5 100644
--- a/setup.c
+++ b/setup.c
@@ -3,6 +3,7 @@
 #include "config.h"
 #include "dir.h"
 #include "string-list.h"
+#include "refs.h"
 
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
-- 
2.17.0.rc1.439.gca064e2955

[PATCH 4/8] setup.c: introduce setup_adjust_path()

[Nguyễn Thái Ngọc Duy]

From: Duy Nguyen 

When $CWD is moved, relative path must be updated to be relative to
the new $CWD. This function helps do that. The _in_place version is
just for convenient (and we will use it quite often in subsequent
patches).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 cache.h |  3 +++
 setup.c | 20 
 2 files changed, 23 insertions(+)

diff --git a/cache.h b/cache.h
index bbaf5c349a..05f32c9659 100644
--- a/cache.h
+++ b/cache.h
@@ -522,6 +522,9 @@ extern void set_git_work_tree(const char *tree);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
+extern void setup_adjust_path(const char *name, char **path,
+ const char *old_cwd,
+ const char *new_cwd);
 extern void setup_work_tree(void);
 /*
  * Find the commondir and gitdir of the repository that contains the current
diff --git a/setup.c b/setup.c
index 664453fcef..e26f44185e 100644
--- a/setup.c
+++ b/setup.c
@@ -376,6 +376,26 @@ int is_inside_work_tree(void)
return inside_work_tree;
 }
 
+void setup_adjust_path(const char *name, char **path,
+  const char *old_cwd,
+  const char *new_cwd)
+{
+   char *old_path = *path;
+   struct strbuf sb = STRBUF_INIT;
+
+   if (!old_path || is_absolute_path(old_path))
+   return;
+
+   strbuf_addstr(, old_cwd);
+   strbuf_ensure_trailing_dir_sep();
+   strbuf_addstr(, old_path);
+   *path = xstrdup(remove_leading_path(sb.buf, new_cwd));
+   trace_printf_key(_setup_key, "setup: adjust '%s' to %s",
+name, *path);
+   strbuf_release();
+   free(old_path);
+}
+
 void setup_work_tree(void)
 {
const char *work_tree, *git_dir;
-- 
2.17.0.rc1.439.gca064e2955

[PATCH 5/8] setup.c: allow other code to be notified when $CWD moves

[Nguyễn Thái Ngọc Duy]

From: Duy Nguyen 

When the current directory is moved, all relative paths may become
invalid because they are still relative to the old current
directory. At this point in setup_work_tree() many objects have been
initialized and some keep relative paths in their data structure.

$GIT_DIR and $GIT_WORK_TREE for example are the two examples which are
dealt with explicitly in this code: $GIT_WORK_TREE is reset to "." and
set_git_dir() is called the second time with a new relative path.

Introduce a more generic mechanism to let all code get notified and do
the path adjustment themselves instead of pulling all path adjustment
logic in here. The $GIT_DIR and $GIT_WORK_TREE adjustments will
removed from this code in later patches.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 cache.h |  5 +
 setup.c | 27 ++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 05f32c9659..895abe7e7e 100644
--- a/cache.h
+++ b/cache.h
@@ -522,6 +522,11 @@ extern void set_git_work_tree(const char *tree);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
+typedef void (*cwd_updated_fn)(const char *old_cwd,
+  const char *new_cwd,
+  void *cb_data);
+void add_cwd_update_callback(cwd_updated_fn fn, void *cb_data);
+void removet_cwd_update_callback(cwd_updated_fn fn, void *cb_data);
 extern void setup_adjust_path(const char *name, char **path,
  const char *old_cwd,
  const char *new_cwd);
diff --git a/setup.c b/setup.c
index e26f44185e..e340ee2130 100644
--- a/setup.c
+++ b/setup.c
@@ -376,6 +376,24 @@ int is_inside_work_tree(void)
return inside_work_tree;
 }
 
+struct cwd_update_callback {
+   cwd_updated_fn fn;
+   void *cb_data;
+};
+
+static struct cwd_update_callback *cwd_callbacks;
+static int nr_cwd_callbacks;
+
+void add_cwd_update_callback(cwd_updated_fn fn, void *cb_data)
+{
+   struct cwd_update_callback *cb;
+
+   REALLOC_ARRAY(cwd_callbacks, nr_cwd_callbacks + 1);
+   cb = cwd_callbacks + nr_cwd_callbacks++;
+   cb->fn = fn;
+   cb->cb_data = cb_data;
+}
+
 void setup_adjust_path(const char *name, char **path,
   const char *old_cwd,
   const char *new_cwd)
@@ -398,8 +416,10 @@ void setup_adjust_path(const char *name, char **path,
 
 void setup_work_tree(void)
 {
-   const char *work_tree, *git_dir;
static int initialized = 0;
+   const char *work_tree, *git_dir;
+   struct strbuf old_cwd = STRBUF_INIT;
+   int i;
 
if (initialized)
return;
@@ -411,6 +431,7 @@ void setup_work_tree(void)
git_dir = get_git_dir();
if (!is_absolute_path(git_dir))
git_dir = real_path(get_git_dir());
+   strbuf_get_pwd_cwd(_cwd);
if (!work_tree || chdir(work_tree))
die(_("this operation must be run in a work tree"));
 
@@ -421,6 +442,10 @@ void setup_work_tree(void)
if (getenv(GIT_WORK_TREE_ENVIRONMENT))
setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
 
+   for (i = 0; i < nr_cwd_callbacks; i++)
+   cwd_callbacks[i].fn(old_cwd.buf, work_tree,
+   cwd_callbacks[i].cb_data);
+   strbuf_release(_cwd);
set_git_dir(remove_leading_path(git_dir, work_tree));
initialized = 1;
 }
-- 
2.17.0.rc1.439.gca064e2955

[PATCH 3/8] trace.c: export trace_setup_key

[Nguyễn Thái Ngọc Duy]

This is so that we can print traces based on this key outside trace.c.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 trace.c | 14 +++---
 trace.h |  1 +
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/trace.c b/trace.c
index 7f3b08e148..fc623e91fd 100644
--- a/trace.c
+++ b/trace.c
@@ -26,6 +26,7 @@
 
 struct trace_key trace_default_key = { "GIT_TRACE", 0, 0, 0 };
 struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
+struct trace_key trace_setup_key = TRACE_KEY_INIT(SETUP);
 
 /* Get a trace file descriptor from "key" env variable. */
 static int get_trace_fd(struct trace_key *key)
@@ -300,11 +301,10 @@ static const char *quote_crnl(const char *path)
 /* FIXME: move prefix to startup_info struct and get rid of this arg */
 void trace_repo_setup(const char *prefix)
 {
-   static struct trace_key key = TRACE_KEY_INIT(SETUP);
const char *git_work_tree;
char *cwd;
 
-   if (!trace_want())
+   if (!trace_want(_setup_key))
return;
 
cwd = xgetcwd();
@@ -315,11 +315,11 @@ void trace_repo_setup(const char *prefix)
if (!prefix)
prefix = "(null)";
 
-   trace_printf_key(, "setup: git_dir: %s\n", 
quote_crnl(get_git_dir()));
-   trace_printf_key(, "setup: git_common_dir: %s\n", 
quote_crnl(get_git_common_dir()));
-   trace_printf_key(, "setup: worktree: %s\n", 
quote_crnl(git_work_tree));
-   trace_printf_key(, "setup: cwd: %s\n", quote_crnl(cwd));
-   trace_printf_key(, "setup: prefix: %s\n", quote_crnl(prefix));
+   trace_printf_key(_setup_key, "setup: git_dir: %s\n", 
quote_crnl(get_git_dir()));
+   trace_printf_key(_setup_key, "setup: git_common_dir: %s\n", 
quote_crnl(get_git_common_dir()));
+   trace_printf_key(_setup_key, "setup: worktree: %s\n", 
quote_crnl(git_work_tree));
+   trace_printf_key(_setup_key, "setup: cwd: %s\n", quote_crnl(cwd));
+   trace_printf_key(_setup_key, "setup: prefix: %s\n", 
quote_crnl(prefix));
 
free(cwd);
 }
diff --git a/trace.h b/trace.h
index 88055abef7..2b6a1bc17c 100644
--- a/trace.h
+++ b/trace.h
@@ -15,6 +15,7 @@ extern struct trace_key trace_default_key;
 
 #define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
 extern struct trace_key trace_perf_key;
+extern struct trace_key trace_setup_key;
 
 extern void trace_repo_setup(const char *prefix);
 extern int trace_want(struct trace_key *key);
-- 
2.17.0.rc1.439.gca064e2955

[PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix)

[Nguyễn Thái Ngọc Duy]

From: Duy Nguyen 

This function was added in 10c4c881c4 (Allow add_path() to add
non-existent directories to the path - 2008-07-21) because getcwd()
may fail on non-existing cwd and we want to construct non-existing
absolute paths sometimes.

The function was merged back in strbuf_add_absolute_path() some time
later. Move it out again because it will have another caller shortly.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 strbuf.c | 37 ++---
 strbuf.h |  6 ++
 2 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index d5b7cda61e..aed4bec856 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -746,27 +746,34 @@ void strbuf_humanise_bytes(struct strbuf *buf, off_t 
bytes)
}
 }
 
+void strbuf_get_pwd_cwd(struct strbuf *sb)
+{
+   struct stat cwd_stat, pwd_stat;
+   char *cwd = xgetcwd();
+   char *pwd = getenv("PWD");
+
+   if (pwd && strcmp(pwd, cwd) &&
+   !stat(cwd, _stat) &&
+   (cwd_stat.st_dev || cwd_stat.st_ino) &&
+   !stat(pwd, _stat) &&
+   pwd_stat.st_dev == cwd_stat.st_dev &&
+   pwd_stat.st_ino == cwd_stat.st_ino)
+   strbuf_addstr(sb, pwd);
+   else
+   strbuf_addstr(sb, cwd);
+   free(cwd);
+}
+
 void strbuf_add_absolute_path(struct strbuf *sb, const char *path)
 {
if (!*path)
die("The empty string is not a valid path");
if (!is_absolute_path(path)) {
-   struct stat cwd_stat, pwd_stat;
size_t orig_len = sb->len;
-   char *cwd = xgetcwd();
-   char *pwd = getenv("PWD");
-   if (pwd && strcmp(pwd, cwd) &&
-   !stat(cwd, _stat) &&
-   (cwd_stat.st_dev || cwd_stat.st_ino) &&
-   !stat(pwd, _stat) &&
-   pwd_stat.st_dev == cwd_stat.st_dev &&
-   pwd_stat.st_ino == cwd_stat.st_ino)
-   strbuf_addstr(sb, pwd);
-   else
-   strbuf_addstr(sb, cwd);
-   if (sb->len > orig_len && !is_dir_sep(sb->buf[sb->len - 1]))
-   strbuf_addch(sb, '/');
-   free(cwd);
+
+   strbuf_get_pwd_cwd(sb);
+   if (sb->len > orig_len)
+   strbuf_ensure_trailing_dir_sep(sb);
}
strbuf_addstr(sb, path);
 }
diff --git a/strbuf.h b/strbuf.h
index 62dc7f16fa..f712c4ff92 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -458,6 +458,12 @@ extern int strbuf_getwholeline_fd(struct strbuf *, int, 
int);
  */
 extern int strbuf_getcwd(struct strbuf *sb);
 
+/**
+ * Return the current directory, fall back to $PWD if the
+ * current directory does not exist.
+ */
+extern void strbuf_get_pwd_cwd(struct strbuf *sb);
+
 /**
  * Add a path to a buffer, converting a relative path to an
  * absolute one in the process.  Symbolic links are not
-- 
2.17.0.rc1.439.gca064e2955

[PATCH 1/8] strbuf.c: add strbuf_ensure_trailing_dr_sep()

[Nguyễn Thái Ngọc Duy]

From: Duy Nguyen 

This is just good cleanup and the logic will also be needed in new
patches.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 abspath.c  | 4 +---
 builtin/difftool.c | 6 ++
 dir-iterator.c | 3 +--
 path.c | 9 +++--
 strbuf.c   | 6 ++
 strbuf.h   | 2 ++
 6 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/abspath.c b/abspath.c
index 9857985329..994075b5c8 100644
--- a/abspath.c
+++ b/abspath.c
@@ -122,9 +122,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char 
*path,
continue;
}
 
-   /* append the next component and resolve resultant path */
-   if (!is_dir_sep(resolved->buf[resolved->len - 1]))
-   strbuf_addch(resolved, '/');
+   strbuf_ensure_trailing_dir_sep(resolved);
strbuf_addbuf(resolved, );
 
if (lstat(resolved->buf, )) {
diff --git a/builtin/difftool.c b/builtin/difftool.c
index ee8dce019e..8d125c7968 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -88,8 +88,7 @@ static int parse_index_info(char *p, int *mode1, int *mode2,
 static void add_path(struct strbuf *buf, size_t base_len, const char *path)
 {
strbuf_setlen(buf, base_len);
-   if (buf->len && buf->buf[buf->len - 1] != '/')
-   strbuf_addch(buf, '/');
+   strbuf_ensure_trailing_dir_sep(buf);
strbuf_addstr(buf, path);
 }
 
@@ -362,8 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
strbuf_addf(, "%s/left/", tmpdir);
strbuf_addf(, "%s/right/", tmpdir);
strbuf_addstr(, workdir);
-   if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
-   strbuf_addch(, '/');
+   strbuf_ensure_trailing_dir_sep();
mkdir(ldir.buf, 0700);
mkdir(rdir.buf, 0700);
 
diff --git a/dir-iterator.c b/dir-iterator.c
index 34182a9a1c..249b5325cf 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -65,8 +65,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
 * Note: dir_iterator_begin() ensures that
 * path is not the empty string.
 */
-   if (!is_dir_sep(iter->base.path.buf[iter->base.path.len 
- 1]))
-   strbuf_addch(>base.path, '/');
+   strbuf_ensure_trailing_dir_sep(>base.path);
level->prefix_len = iter->base.path.len;
 
level->dir = opendir(iter->base.path.buf);
diff --git a/path.c b/path.c
index 3308b7b958..cd0ad89868 100644
--- a/path.c
+++ b/path.c
@@ -408,8 +408,7 @@ static void do_git_path(const struct repository *repo,
 {
int gitdir_len;
strbuf_worktree_gitdir(buf, repo, wt);
-   if (buf->len && !is_dir_sep(buf->buf[buf->len - 1]))
-   strbuf_addch(buf, '/');
+   strbuf_ensure_trailing_dir_sep(buf);
gitdir_len = buf->len;
strbuf_vaddf(buf, fmt, args);
if (!wt)
@@ -512,8 +511,7 @@ static void do_worktree_path(const struct repository *repo,
 const char *fmt, va_list args)
 {
strbuf_addstr(buf, repo->worktree);
-   if(buf->len && !is_dir_sep(buf->buf[buf->len - 1]))
-   strbuf_addch(buf, '/');
+   strbuf_ensure_trailing_dir_sep(buf);
 
strbuf_vaddf(buf, fmt, args);
strbuf_cleanup_path(buf);
@@ -608,8 +606,7 @@ static void do_git_common_path(const struct repository 
*repo,
   va_list args)
 {
strbuf_addstr(buf, repo->commondir);
-   if (buf->len && !is_dir_sep(buf->buf[buf->len - 1]))
-   strbuf_addch(buf, '/');
+   strbuf_ensure_trailing_dir_sep(buf);
strbuf_vaddf(buf, fmt, args);
strbuf_cleanup_path(buf);
 }
diff --git a/strbuf.c b/strbuf.c
index 83d05024e6..d5b7cda61e 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -122,6 +122,12 @@ void strbuf_ltrim(struct strbuf *sb)
sb->buf[sb->len] = '\0';
 }
 
+void strbuf_ensure_trailing_dir_sep(struct strbuf *sb)
+{
+   if (sb->len && !is_dir_sep(sb->buf[sb->len - 1]))
+   strbuf_addch(sb, '/');
+}
+
 int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)
 {
char *out;
diff --git a/strbuf.h b/strbuf.h
index c4de5e4588..62dc7f16fa 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -189,6 +189,8 @@ extern void strbuf_ltrim(struct strbuf *);
 
 /* Strip trailing directory separators */
 extern void strbuf_trim_trailing_dir_sep(struct strbuf *);
+/* Append trailing directory separator if necessary */
+extern void strbuf_ensure_trailing_dir_sep(struct strbuf *sb);
 
 /**
  * Replace the contents of the strbuf with a reencoded form.  Returns -1
-- 
2.17.0.rc1.439.gca064e2955

[PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Nguyễn Thái Ngọc Duy]

On Wed, Mar 28, 2018 at 7:36 PM, Jeff King  wrote:
> On Wed, Mar 28, 2018 at 12:10:21PM +0200, Duy Nguyen wrote:
>
>> > I think it might be clearer if a single call is given both the old and
>> > new paths. That requires the caller of chdir() storing getcwd() before
>> > it moves, but I don't think that should be a big deal.
>>
>> The problem is switching relative paths relies on the old $CWD if I'm
>> not mistaken and we need  getcwd() for this. I'd love to have one
>> callback that says "$CWD has been switched from this path to that
>> path, do whatever you need to" that can be called any time, before or
>> after chdir(). I'll look more into it.
>
> I think it should be OK to save getcwd() and just construct the original
> path after the fact. Here's some patches which do that in a nice way.

Heh.. I should have checked mails more often while coding ;-)

This is what I got, which is slightly different from your series
because I want to call set_git_dir() just one time (by
setup_git_directory) and never again. I think the API looks close
enough.

I will probably rework on top of your chdir-notify instead (and let
yours to be merged earlier)

Note, this one is built on a strange base, which is a merge of 'next'
and 'sb/object-store' (I needed 'next' and Junio would have another
evil merge if 'sb/object-store' was not in the base).

Nguyễn Thái Ngọc Duy (8):
  strbuf.c: add strbuf_ensure_trailing_dr_sep()
  strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix)
  trace.c: export trace_setup_key
  setup.c: introduce setup_adjust_path()
  setup.c: allow other code to be notified when $CWD moves
  environment.c: adjust env containing relpath when $CWD is moved
  repository: adjust repo paths when $CWD moves
  refs: adjust main repo paths when $CWD moves

 abspath.c |  4 +--
 builtin/difftool.c|  6 ++---
 cache.h   |  8 ++
 dir-iterator.c|  3 +--
 environment.c | 46 +
 object-store.h|  3 +++
 object.c  | 15 +++
 path.c|  9 +++
 refs.c| 10 
 refs/files-backend.c  | 15 +++
 refs/packed-backend.c | 12 +
 refs/refs-internal.h  |  4 +++
 repository.c  | 28 
 setup.c   | 59 ++-
 strbuf.c  | 43 ---
 strbuf.h  |  8 ++
 trace.c   | 14 +-
 trace.h   |  1 +
 18 files changed, 239 insertions(+), 49 deletions(-)

-- 
2.17.0.rc1.439.gca064e2955

Re: [PATCHv2 0/6] Moving submodules with nested submodules

[Jonathan Tan]

On Wed, 28 Mar 2018 10:24:43 -0700
Stefan Beller  wrote:

> * picked up Jonathans patch and added it as a nice finish of the series.
>   I did not see the need or aesthetic desire to put that patch earlier
>   in the series.

Thanks for picking up my patch. The aesthetic desire is to avoid what's
currently happening in PATCH 2/6 version 2, where we still have the
potentially confusing submodule_free() -> submodule_free(repo)
conversion, which is also later redone in PATCH 6/6 version 2
(submodule_free(repo) -> submodule_free(the_repository)). But I'll leave
the final decision to you.

Other than that, besides PATCH 5/6 (which I have already commented on),
everything looks good.

Re: [PATCH 5/6] submodule: fixup nested submodules after moving the submodule

[Jonathan Tan]

On Wed, 28 Mar 2018 10:24:48 -0700
Stefan Beller  wrote:

> +static void connect_wt_gitdir_in_nested(const char *sub_worktree,
> + const char *sub_gitdir)
> +{
> + int i;
> + struct repository subrepo;
> + struct strbuf sub_wt = STRBUF_INIT;
> + struct strbuf sub_gd = STRBUF_INIT;
> +
> + const struct submodule *sub;
> +
> + if (repo_init(, sub_gitdir, sub_worktree))
> + return;

If repo_init() fails, is it because the working tree doesn't exist on
disk, so we don't need to perform any connections on submodules? I think
a comment should be added to describe this.

> +
> + if (repo_read_index() < 0)
> + die("index file corrupt in repo %s", subrepo.gitdir);
> +
> + for (i = 0; i < subrepo.index->cache_nr; i++) {
> + const struct cache_entry *ce = subrepo.index->cache[i];
> +
> + if (!S_ISGITLINK(ce->ce_mode))
> + continue;
> +
> + while (i + 1 < subrepo.index->cache_nr &&
> +!strcmp(ce->name, subrepo.index->cache[i + 1]->name))
> + /*
> +  * Skip entries with the same name in different stages
> +  * to make sure an entry is returned only once.
> +  */
> + i++;
> +
> + sub = submodule_from_path(, _oid, ce->name);
> + if (!sub)
> + /* submodule not checked out? */
> + continue;
> +
> + if (is_submodule_active(, ce->name)) {

Optional: This could be combined with the previous "if" block, so that
the following lines don't need to be indented.

> + strbuf_addf(_wt, "%s/%s", sub_worktree, sub->path);
> + strbuf_addf(_gd, "%s/modules/%s", sub_gitdir, 
> sub->name);
> +
> + connect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 
> 0);
> + connect_wt_gitdir_in_nested(sub_wt.buf, sub_gd.buf);

What's the difference between having this call to
connect_wt_gitdir_in_nested() and just passing 1 instead of 0 to
connect_work_tree_and_git_dir()? I see that the latter uses absolute
paths, but that would seem to have the same effect. (If not, I think a
comment is warranted.)

> +
> + strbuf_reset(_wt);
> + strbuf_reset(_gd);

I think we normally write the resets before the strbuf_addf(), so that
it's clearer that sub_wt and sub_gd are meant to start from scratch in
every iteration.

Overall, this version of the patch is clearer - thanks.

[PATCH 4/4] refs: use chdir_notify to update cached relative paths

[Jeff King]

Commit f57f37e2e1 (files-backend: remove the use of
git_path(), 2017-03-26) introduced a regression when a
relative $GIT_DIR is used in a working tree:

  - when we initialize the ref backend, we make a copy of
get_git_dir(), which may be relative

  - later, we may call setup_work_tree() and chdir to the
root of the working tree

  - further calls to the ref code will use the stored git
directory, but relative paths will now point to the
wrong place

The new test in t1501 demonstrates one such instance (the
bug causes us to write the ref update to the nonsense
"relative/relative/.git").

Since setup_work_tree() now uses chdir_notify, we can just
ask it update our relative paths when necessary.

Reported-by: Rafael Ascensao 
Helped-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Jeff King 
---
 refs/files-backend.c  |  4 
 refs/packed-backend.c |  3 +++
 t/t1501-work-tree.sh  | 12 
 3 files changed, 19 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index bec8e30e9e..ab3e00ffa0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -9,6 +9,7 @@
 #include "../lockfile.h"
 #include "../object.h"
 #include "../dir.h"
+#include "../chdir-notify.h"
 
 /*
  * This backend uses the following flags in `ref_update::flags` for
@@ -106,6 +107,9 @@ static struct ref_store *files_ref_store_create(const char 
*gitdir,
refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
strbuf_release();
 
+   chdir_notify_reparent(>gitdir);
+   chdir_notify_reparent(>gitcommondir);
+
return ref_store;
 }
 
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 65288c6472..6725742f00 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -5,6 +5,7 @@
 #include "packed-backend.h"
 #include "../iterator.h"
 #include "../lockfile.h"
+#include "../chdir-notify.h"
 
 enum mmap_strategy {
/*
@@ -202,6 +203,8 @@ struct ref_store *packed_ref_store_create(const char *path,
refs->store_flags = store_flags;
 
refs->path = xstrdup(path);
+   chdir_notify_reparent(>path);
+
return ref_store;
 }
 
diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
index b06210ec5e..a5db53337b 100755
--- a/t/t1501-work-tree.sh
+++ b/t/t1501-work-tree.sh
@@ -431,4 +431,16 @@ test_expect_success 'error out gracefully on invalid 
$GIT_WORK_TREE' '
)
 '
 
+test_expect_success 'refs work with relative gitdir and work tree' '
+   git init relative &&
+   git -C relative commit --allow-empty -m one &&
+   git -C relative commit --allow-empty -m two &&
+
+   GIT_DIR=relative/.git GIT_WORK_TREE=relative git reset HEAD^ &&
+
+   git -C relative log -1 --format=%s >actual &&
+   echo one >expect &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.17.0.rc1.515.g3cc84c0ca4

[PATCH 3/4] set_work_tree: use chdir_notify

[Jeff King]

When we change to the top of the working tree, we manually
re-adjust $GIT_DIR and call set_git_dir() again, in order to
update any relative git-dir we'd compute earlier.

Instead of the work-tree code having to know to call the
git-dir code, let's use the new chdir_notify interface.
There are two spots that need updating, with a few
subtleties in each:

  1. the set_git_dir() code needs to chdir_notify_register()
 so it can be told when to update its path.

 Technically we could push this down into repo_set_gitdir(),
 so that even repository structs besides the_repository
 could benefit from this. But that opens up a lot of
 complications:

  - we'd still need to touch set_git_dir(), because it
does some other setup (like setting $GIT_DIR in the
environment)

  - submodules using other repository structs get
cleaned up, which means we'd need to remove them
from the chdir_notify list

  - it's unlikely to fix any bugs, since we shouldn't
generally chdir() in the middle of working on a
submodule

  2. setup_work_tree now needs to call chdir_notify(), and
 can lose its manual set_git_dir() call.

 Note that at first glance it looks like this undoes the
 absolute-to-relative optimization added by 044bbbcb63
 (Make git_dir a path relative to work_tree in
 setup_work_tree(), 2008-06-19). But for the most part
 that optimization was just _undoing_ the
 relative-to-absolute conversion which the function was
 doing earlier (and which is now gone).

 It is true that if you already have an absolute git_dir
 that the setup_work_tree() function will no longer make
 it relative as a side effect. But:

   - we generally do have relative git-dir's due to the
 way the discovery code works

   - if we really care about making git-dir's relative
 when possible, then we should be relativizing them
 earlier (e.g., when we see an absolute $GIT_DIR we
 could turn it relative, whether we are going to
 chdir into a worktree or not). That would cover all
 cases, including ones that 044bbbcb63 did not.

Signed-off-by: Jeff King 
---
 environment.c | 19 ++-
 setup.c   |  9 +++--
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/environment.c b/environment.c
index e01acf8b11..e1f535ae08 100644
--- a/environment.c
+++ b/environment.c
@@ -13,6 +13,7 @@
 #include "refs.h"
 #include "fmt-merge-msg.h"
 #include "commit.h"
+#include "chdir-notify.h"
 
 int trust_executable_bit = 1;
 int trust_ctime = 1;
@@ -296,7 +297,7 @@ char *get_graft_file(void)
return the_repository->graft_file;
 }
 
-void set_git_dir(const char *path)
+static void set_git_dir_1(const char *path)
 {
if (setenv(GIT_DIR_ENVIRONMENT, path, 1))
die("could not set GIT_DIR to '%s'", path);
@@ -304,6 +305,22 @@ void set_git_dir(const char *path)
setup_git_env();
 }
 
+static void update_relative_gitdir(const char *old_cwd,
+  const char *new_cwd,
+  void *data)
+{
+   char *path = reparent_relative_path(old_cwd, new_cwd, get_git_dir());
+   set_git_dir_1(path);
+   free(path);
+}
+
+void set_git_dir(const char *path)
+{
+   set_git_dir_1(path);
+   if (!is_absolute_path(path))
+   chdir_notify_register(update_relative_gitdir, NULL);
+}
+
 const char *get_log_output_encoding(void)
 {
return git_log_output_encoding ? git_log_output_encoding
diff --git a/setup.c b/setup.c
index 7287779642..9eb2e808e1 100644
--- a/setup.c
+++ b/setup.c
@@ -3,6 +3,7 @@
 #include "config.h"
 #include "dir.h"
 #include "string-list.h"
+#include "chdir-notify.h"
 
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
@@ -378,7 +379,7 @@ int is_inside_work_tree(void)
 
 void setup_work_tree(void)
 {
-   const char *work_tree, *git_dir;
+   const char *work_tree;
static int initialized = 0;
 
if (initialized)
@@ -388,10 +389,7 @@ void setup_work_tree(void)
die(_("unable to set up work tree using invalid config"));
 
work_tree = get_git_work_tree();
-   git_dir = get_git_dir();
-   if (!is_absolute_path(git_dir))
-   git_dir = real_path(get_git_dir());
-   if (!work_tree || chdir(work_tree))
+   if (!work_tree || chdir_notify(work_tree))
die(_("this operation must be run in a work tree"));
 
/*
@@ -401,7 +399,6 @@ void setup_work_tree(void)
if (getenv(GIT_WORK_TREE_ENVIRONMENT))
setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
 
-   set_git_dir(remove_leading_path(git_dir, work_tree));
initialized = 1;
 }
 
-- 
2.17.0.rc1.515.g3cc84c0ca4

[PATCH 2/4] add chdir-notify API

[Jeff King]

If one part of the code does a permanent chdir(), then this
invalidates any relative paths that may be held by other
parts of the code. For example, setup_work_tree() moves us
to the top of the working tree, which may invalidate a
previously stored relative gitdir.

We've hacked around this case by teaching setup_work_tree()
to re-run set_git_dir() with an adjusted path, but this
stomps all over the idea of module boundaries.
setup_work_tree() shouldn't have to know all of the places
that need to be fed an adjusted path. And indeed, there's at
least one other place (the refs code) which needs adjusting.

Let's provide an API to let code that stores relative paths
"subscribe" to updates to the current working directory.
This means that callers of chdir() don't need to know about
all subscribers ahead of time; they can simply consult a
dynamically built list.

Note that our helper function to reparent relative paths
uses the simple remove_leading_path(). We could in theory
use the much smarter relative_path(), but that led to some
problems as described in 41894ae3a3 (Use simpler
relative_path when set_git_dir, 2013-10-14). Since we're
aiming to replace the setup_work_tree() code here, let's
follow its lead.

Signed-off-by: Jeff King 
---
 Makefile   |  1 +
 chdir-notify.c | 71 ++
 chdir-notify.h | 64 +
 3 files changed, 136 insertions(+)
 create mode 100644 chdir-notify.c
 create mode 100644 chdir-notify.h

diff --git a/Makefile b/Makefile
index a1d8775adb..0da98b9274 100644
--- a/Makefile
+++ b/Makefile
@@ -772,6 +772,7 @@ LIB_OBJS += branch.o
 LIB_OBJS += bulk-checkin.o
 LIB_OBJS += bundle.o
 LIB_OBJS += cache-tree.o
+LIB_OBJS += chdir-notify.o
 LIB_OBJS += checkout.o
 LIB_OBJS += color.o
 LIB_OBJS += column.o
diff --git a/chdir-notify.c b/chdir-notify.c
new file mode 100644
index 00..0b88bf0e5b
--- /dev/null
+++ b/chdir-notify.c
@@ -0,0 +1,71 @@
+#include "cache.h"
+#include "chdir-notify.h"
+#include "list.h"
+#include "strbuf.h"
+
+struct chdir_notify_entry {
+   chdir_notify_callback cb;
+   void *data;
+   struct list_head list;
+};
+static LIST_HEAD(chdir_notify_entries);
+
+void chdir_notify_register(chdir_notify_callback cb, void *data)
+{
+   struct chdir_notify_entry *e = xmalloc(sizeof(*e));
+   e->cb = cb;
+   e->data = data;
+   list_add_tail(>list, _notify_entries);
+}
+
+static void reparent_cb(const char *old_cwd,
+   const char *new_cwd,
+   void *data)
+{
+   char **path = data;
+   char *tmp = *path;
+   *path = reparent_relative_path(old_cwd, new_cwd, tmp);
+   free(tmp);
+}
+
+void chdir_notify_reparent(char **path)
+{
+   if (!is_absolute_path(*path))
+   chdir_notify_register(reparent_cb, path);
+}
+
+int chdir_notify(const char *new_cwd)
+{
+   struct strbuf old_cwd = STRBUF_INIT;
+   struct list_head *pos;
+
+   if (strbuf_getcwd(_cwd) < 0)
+   return -1;
+   if (chdir(new_cwd) < 0)
+   return -1;
+
+   list_for_each(pos, _notify_entries) {
+   struct chdir_notify_entry *e =
+   list_entry(pos, struct chdir_notify_entry, list);
+   e->cb(old_cwd.buf, new_cwd, e->data);
+   }
+
+   strbuf_release(_cwd);
+   return 0;
+}
+
+char *reparent_relative_path(const char *old_cwd,
+const char *new_cwd,
+const char *path)
+{
+   char *ret, *full;
+
+   if (is_absolute_path(path))
+   return xstrdup(path);
+
+   full = xstrfmt("%s/%s", old_cwd, path);
+   ret = xstrdup(remove_leading_path(full, new_cwd));
+   free(full);
+
+   return ret;
+}
diff --git a/chdir-notify.h b/chdir-notify.h
new file mode 100644
index 00..c3bd818a85
--- /dev/null
+++ b/chdir-notify.h
@@ -0,0 +1,64 @@
+#ifndef CHDIR_NOTIFY_H
+#define CHDIR_NOTIFY_H
+
+/*
+ * An API to let code "subscribe" to changes to the current working directory.
+ * The general idea is that some code asks to be notified when the working
+ * directory changes, and other code that calls chdir uses a special wrapper
+ * that notifies everyone.
+ */
+
+/*
+ * Callers who need to know about changes can do:
+ *
+ *   void foo(const char *old_path, const char *new_path, void *data)
+ *   {
+ * warning("switched from %s to %s!", old_path, new_path);
+ *   }
+ *   ...
+ *   chdir_notify_register(foo, data);
+ *
+ * In practice most callers will want to move a relative path to the new root;
+ * they can use the reparent_relative_path() helper for that. If that's all
+ * you're doing, you can also use the convenience function:
+ *
+ *   chdir_notify_reparent(_path);
+ *
+ * Registered functions are called in the order in which they were added. Note
+ * that there's currently no way to remove a function, so make sure that the
+ * data 

[PATCH 1/4] set_git_dir: die when setenv() fails

[Jeff King]

The set_git_dir() function returns an error if setenv()
fails, but there are zero callers who pay attention to this
return value. If this ever were to happen, it could cause
confusing results, as sub-processes would see a potentially
stale GIT_DIR (e.g., if it is relative and we chdir()-ed to
the root of the working tree).

We _could_ try to fix each caller, but there's really
nothing useful to do after this failure except die. Let's
just lump setenv() failure into the same category as malloc
failure: things that should never happen and cause us to
abort catastrophically.

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

diff --git a/cache.h b/cache.h
index a61b2d3f0d..5c24394d84 100644
--- a/cache.h
+++ b/cache.h
@@ -477,7 +477,7 @@ extern const char *get_git_common_dir(void);
 extern char *get_object_directory(void);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
-extern int set_git_dir(const char *path);
+extern void set_git_dir(const char *path);
 extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 extern int get_common_dir(struct strbuf *sb, const char *gitdir);
 extern const char *get_git_namespace(void);
diff --git a/environment.c b/environment.c
index d6dd64662c..e01acf8b11 100644
--- a/environment.c
+++ b/environment.c
@@ -296,13 +296,12 @@ char *get_graft_file(void)
return the_repository->graft_file;
 }
 
-int set_git_dir(const char *path)
+void set_git_dir(const char *path)
 {
if (setenv(GIT_DIR_ENVIRONMENT, path, 1))
-   return error("Could not set GIT_DIR to '%s'", path);
+   die("could not set GIT_DIR to '%s'", path);
repo_set_gitdir(the_repository, path);
setup_git_env();
-   return 0;
 }
 
 const char *get_log_output_encoding(void)
-- 
2.17.0.rc1.515.g3cc84c0ca4

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King]

On Wed, Mar 28, 2018 at 12:10:21PM +0200, Duy Nguyen wrote:

> > I think it might be clearer if a single call is given both the old and
> > new paths. That requires the caller of chdir() storing getcwd() before
> > it moves, but I don't think that should be a big deal.
> 
> The problem is switching relative paths relies on the old $CWD if I'm
> not mistaken and we need  getcwd() for this. I'd love to have one
> callback that says "$CWD has been switched from this path to that
> path, do whatever you need to" that can be called any time, before or
> after chdir(). I'll look more into it.

I think it should be OK to save getcwd() and just construct the original
path after the fact. Here's some patches which do that in a nice way.

For those just joining us, this fixes a regression that was in v2.13 (so
nothing we need to worry about as part of the 2.17-rc track).

  [1/4]: set_git_dir: die when setenv() fails
  [2/4]: add chdir-notify API
  [3/4]: set_work_tree: use chdir_notify
  [4/4]: refs: use chdir_notify to update cached relative paths

 Makefile  |  1 +
 cache.h   |  2 +-
 chdir-notify.c| 71 +++
 chdir-notify.h| 64 ++
 environment.c | 22 --
 refs/files-backend.c  |  4 +++
 refs/packed-backend.c |  3 ++
 setup.c   |  9 ++
 t/t1501-work-tree.sh  | 12 
 9 files changed, 178 insertions(+), 10 deletions(-)
 create mode 100644 chdir-notify.c
 create mode 100644 chdir-notify.h

-Peff

Re: [PATCH 5/6] submodule: fixup nested submodules after moving the submodule

[Brandon Williams]

On 03/28, Stefan Beller wrote:
> connect_work_tree_and_git_dir is used to connect a submodule worktree with
> its git directory and vice versa after events that require a reconnection
> such as moving around the working tree. As submodules can have nested
> submodules themselves, we'd also want to fix the nested submodules when
> asked to. Add an option to recurse into the nested submodules and connect
> them as well.
> 
> As submodules are identified by their name (which determines their git
> directory in relation to their superproject's git directory) internally
> and by their path in the working tree of the superproject, we need to
> make sure that the mapping of name <-> path is kept intact. We can do
> that in the git-mv command by writing out the gitmodules file first
> and then forcing a reload of the submodule config machinery.
> 
> Signed-off-by: Stefan Beller 
> ---
>  builtin/mv.c|  6 ++--
>  builtin/submodule--helper.c |  3 +-
>  dir.c   | 63 +++--
>  dir.h   | 12 ++-
>  submodule.c |  6 ++--
>  t/t7001-mv.sh   |  2 +-
>  6 files changed, 80 insertions(+), 12 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 6d141f7a53..7a63667d64 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -276,10 +276,12 @@ int cmd_mv(int argc, const char **argv, const char 
> *prefix)
>   die_errno(_("renaming '%s' failed"), src);
>   }
>   if (submodule_gitfile[i]) {
> - if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
> - connect_work_tree_and_git_dir(dst, 
> submodule_gitfile[i]);
>   if (!update_path_in_gitmodules(src, dst))
>   gitmodules_modified = 1;
> + if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
> + connect_work_tree_and_git_dir(dst,
> +   
> submodule_gitfile[i],
> +   1);
>   }
>  
>   if (mode == WORKING_DIRECTORY)
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a921fbbf56..05fd657f99 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1259,8 +1259,7 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   strbuf_reset();
>   }
>  
> - /* Connect module worktree and git dir */
> - connect_work_tree_and_git_dir(path, sm_gitdir);
> + connect_work_tree_and_git_dir(path, sm_gitdir, 0);
>  
>   p = git_pathdup_submodule(path, "config");
>   if (!p)
> diff --git a/dir.c b/dir.c
> index dedbf5d476..71947c0ef3 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -19,6 +19,7 @@
>  #include "varint.h"
>  #include "ewah/ewok.h"
>  #include "fsmonitor.h"
> +#include "submodule-config.h"
>  
>  /*
>   * Tells read_directory_recursive how a file or directory should be treated.
> @@ -3010,8 +3011,60 @@ void untracked_cache_add_to_index(struct index_state 
> *istate,
>   untracked_cache_invalidate_path(istate, path, 1);
>  }
>  
> -/* Update gitfile and core.worktree setting to connect work tree and git dir 
> */
> -void connect_work_tree_and_git_dir(const char *work_tree_, const char 
> *git_dir_)
> +static void connect_wt_gitdir_in_nested(const char *sub_worktree,
> + const char *sub_gitdir)
> +{
> + int i;
> + struct repository subrepo;
> + struct strbuf sub_wt = STRBUF_INIT;
> + struct strbuf sub_gd = STRBUF_INIT;
> +
> + const struct submodule *sub;
> +
> + if (repo_init(, sub_gitdir, sub_worktree))
> + return;

Note that in Duy's object-store series he made this function private
(IIRC) so this will result in some clash of the two series.

> +
> + if (repo_read_index() < 0)
> + die("index file corrupt in repo %s", subrepo.gitdir);
> +
> + for (i = 0; i < subrepo.index->cache_nr; i++) {
> + const struct cache_entry *ce = subrepo.index->cache[i];
> +
> + if (!S_ISGITLINK(ce->ce_mode))
> + continue;
> +
> + while (i + 1 < subrepo.index->cache_nr &&
> +!strcmp(ce->name, subrepo.index->cache[i + 1]->name))
> + /*
> +  * Skip entries with the same name in different stages
> +  * to make sure an entry is returned only once.
> +  */
> + i++;
> +
> + sub = submodule_from_path(, _oid, ce->name);
> + if (!sub)
> + /* submodule not checked out? */
> + continue;
> +
> + if (is_submodule_active(, ce->name)) {
> + strbuf_addf(_wt, "%s/%s", sub_worktree, sub->path);
> + 

[PATCH 6/6] grep: remove "repo" arg from non-supporting funcs

[Stefan Beller]

From: Jonathan Tan 

As part of commit f9ee2fcdfa ("grep: recurse in-process using 'struct
repository'", 2017-08-02), many functions in builtin/grep.c were
converted to also take "struct repository *" arguments. Among them were
grep_object() and grep_objects().

However, at least grep_objects() was converted incompletely - it calls
gitmodules_config_oid(), which references the_repository.

But it turns out that the conversion was extraneous anyway - there has
been no user-visible effect - because grep_objects() is never invoked
except with the_repository. This is because grepping through objects
cannot be done recursively into submodules.

Revert the changes to grep_objects() and grep_object() (which conversion
is also extraneous) to show that both these functions do not support
repositories other than the_repository.

Signed-off-by: Jonathan Tan 
Signed-off-by: Stefan Beller 
---
 builtin/grep.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 8f04cde18e..091b3f4cc7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -601,8 +601,7 @@ static int grep_tree(struct grep_opt *opt, const struct 
pathspec *pathspec,
 }
 
 static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
-  struct object *obj, const char *name, const char *path,
-  struct repository *repo)
+  struct object *obj, const char *name, const char *path)
 {
if (obj->type == OBJ_BLOB)
return grep_oid(opt, >oid, name, 0, path);
@@ -629,7 +628,7 @@ static int grep_object(struct grep_opt *opt, const struct 
pathspec *pathspec,
}
init_tree_desc(, data, size);
hit = grep_tree(opt, pathspec, , , base.len,
-   obj->type == OBJ_COMMIT, repo);
+   obj->type == OBJ_COMMIT, the_repository);
strbuf_release();
free(data);
return hit;
@@ -638,7 +637,6 @@ static int grep_object(struct grep_opt *opt, const struct 
pathspec *pathspec,
 }
 
 static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,
-   struct repository *repo,
const struct object_array *list)
 {
unsigned int i;
@@ -651,11 +649,11 @@ static int grep_objects(struct grep_opt *opt, const 
struct pathspec *pathspec,
 
/* load the gitmodules file for this rev */
if (recurse_submodules) {
-   submodule_free(repo);
+   submodule_free(the_repository);
gitmodules_config_oid(_obj->oid);
}
-   if (grep_object(opt, pathspec, real_obj, list->objects[i].name, 
list->objects[i].path,
-   repo)) {
+   if (grep_object(opt, pathspec, real_obj, list->objects[i].name,
+   list->objects[i].path)) {
hit = 1;
if (opt->status_only)
break;
@@ -1107,7 +1105,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
if (cached)
die(_("both --cached and trees are given."));
 
-   hit = grep_objects(, , the_repository, );
+   hit = grep_objects(, , );
}
 
if (num_threads)
-- 
2.17.0.rc1.321.gba9d0f2565-goog

[PATCH 4/6] submodule-config: remove submodule_from_cache

[Stefan Beller]

This continues the story of bf12fcdf5e (submodule-config: store
the_submodule_cache in the_repository, 2017-06-22).

The previous patch taught submodule_from_path to take a repository into
account, such that submodule_from_{path, cache} are the same now.
Remove submodule_from_cache, migrating all its callers to
submodule_from_path.

Reviewed-by: Jonathan Tan 
Signed-off-by: Stefan Beller 
---
 repository.c   | 2 +-
 submodule-config.c | 9 -
 submodule-config.h | 3 ---
 submodule.c| 4 ++--
 4 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/repository.c b/repository.c
index 4ffbe9bc94..fa0a132e22 100644
--- a/repository.c
+++ b/repository.c
@@ -167,7 +167,7 @@ int repo_submodule_init(struct repository *submodule,
struct strbuf worktree = STRBUF_INIT;
int ret = 0;
 
-   sub = submodule_from_cache(superproject, _oid, path);
+   sub = submodule_from_path(superproject, _oid, path);
if (!sub) {
ret = -1;
goto out;
diff --git a/submodule-config.c b/submodule-config.c
index 4b7803e6ed..cb65354d4c 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -635,15 +635,6 @@ const struct submodule *submodule_from_path(struct 
repository *r,
return config_from(r->submodule_cache, treeish_name, path, lookup_path);
 }
 
-const struct submodule *submodule_from_cache(struct repository *repo,
-const struct object_id 
*treeish_name,
-const char *key)
-{
-   gitmodules_read_check(repo);
-   return config_from(repo->submodule_cache, treeish_name,
-  key, lookup_path);
-}
-
 void submodule_free(struct repository *r)
 {
if (r->submodule_cache)
diff --git a/submodule-config.h b/submodule-config.h
index 43dfe7dec0..6f686184e8 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -45,9 +45,6 @@ const struct submodule *submodule_from_name(struct repository 
*r,
 const struct submodule *submodule_from_path(struct repository *r,
const struct object_id 
*commit_or_tree,
const char *path);
-extern const struct submodule *submodule_from_cache(struct repository *repo,
-   const struct object_id 
*treeish_name,
-   const char *key);
 void submodule_free(struct repository *r);
 
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index e94b7f9acd..89d0aee086 100644
--- a/submodule.c
+++ b/submodule.c
@@ -230,7 +230,7 @@ int is_submodule_active(struct repository *repo, const char 
*path)
const struct string_list *sl;
const struct submodule *module;
 
-   module = submodule_from_cache(repo, _oid, path);
+   module = submodule_from_path(repo, _oid, path);
 
/* early return if there isn't a path->module mapping */
if (!module)
@@ -1235,7 +1235,7 @@ static int get_next_submodule(struct child_process *cp,
if (!S_ISGITLINK(ce->ce_mode))
continue;
 
-   submodule = submodule_from_cache(spf->r, _oid, ce->name);
+   submodule = submodule_from_path(spf->r, _oid, ce->name);
if (!submodule) {
const char *name = default_name_or_path(ce->name);
if (name) {
-- 
2.17.0.rc1.321.gba9d0f2565-goog

[PATCH 5/6] submodule: fixup nested submodules after moving the submodule

[Stefan Beller]

connect_work_tree_and_git_dir is used to connect a submodule worktree with
its git directory and vice versa after events that require a reconnection
such as moving around the working tree. As submodules can have nested
submodules themselves, we'd also want to fix the nested submodules when
asked to. Add an option to recurse into the nested submodules and connect
them as well.

As submodules are identified by their name (which determines their git
directory in relation to their superproject's git directory) internally
and by their path in the working tree of the superproject, we need to
make sure that the mapping of name <-> path is kept intact. We can do
that in the git-mv command by writing out the gitmodules file first
and then forcing a reload of the submodule config machinery.

Signed-off-by: Stefan Beller 
---
 builtin/mv.c|  6 ++--
 builtin/submodule--helper.c |  3 +-
 dir.c   | 63 +++--
 dir.h   | 12 ++-
 submodule.c |  6 ++--
 t/t7001-mv.sh   |  2 +-
 6 files changed, 80 insertions(+), 12 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 6d141f7a53..7a63667d64 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -276,10 +276,12 @@ int cmd_mv(int argc, const char **argv, const char 
*prefix)
die_errno(_("renaming '%s' failed"), src);
}
if (submodule_gitfile[i]) {
-   if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
-   connect_work_tree_and_git_dir(dst, 
submodule_gitfile[i]);
if (!update_path_in_gitmodules(src, dst))
gitmodules_modified = 1;
+   if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
+   connect_work_tree_and_git_dir(dst,
+ 
submodule_gitfile[i],
+ 1);
}
 
if (mode == WORKING_DIRECTORY)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a921fbbf56..05fd657f99 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1259,8 +1259,7 @@ static int module_clone(int argc, const char **argv, 
const char *prefix)
strbuf_reset();
}
 
-   /* Connect module worktree and git dir */
-   connect_work_tree_and_git_dir(path, sm_gitdir);
+   connect_work_tree_and_git_dir(path, sm_gitdir, 0);
 
p = git_pathdup_submodule(path, "config");
if (!p)
diff --git a/dir.c b/dir.c
index dedbf5d476..71947c0ef3 100644
--- a/dir.c
+++ b/dir.c
@@ -19,6 +19,7 @@
 #include "varint.h"
 #include "ewah/ewok.h"
 #include "fsmonitor.h"
+#include "submodule-config.h"
 
 /*
  * Tells read_directory_recursive how a file or directory should be treated.
@@ -3010,8 +3011,60 @@ void untracked_cache_add_to_index(struct index_state 
*istate,
untracked_cache_invalidate_path(istate, path, 1);
 }
 
-/* Update gitfile and core.worktree setting to connect work tree and git dir */
-void connect_work_tree_and_git_dir(const char *work_tree_, const char 
*git_dir_)
+static void connect_wt_gitdir_in_nested(const char *sub_worktree,
+   const char *sub_gitdir)
+{
+   int i;
+   struct repository subrepo;
+   struct strbuf sub_wt = STRBUF_INIT;
+   struct strbuf sub_gd = STRBUF_INIT;
+
+   const struct submodule *sub;
+
+   if (repo_init(, sub_gitdir, sub_worktree))
+   return;
+
+   if (repo_read_index() < 0)
+   die("index file corrupt in repo %s", subrepo.gitdir);
+
+   for (i = 0; i < subrepo.index->cache_nr; i++) {
+   const struct cache_entry *ce = subrepo.index->cache[i];
+
+   if (!S_ISGITLINK(ce->ce_mode))
+   continue;
+
+   while (i + 1 < subrepo.index->cache_nr &&
+  !strcmp(ce->name, subrepo.index->cache[i + 1]->name))
+   /*
+* Skip entries with the same name in different stages
+* to make sure an entry is returned only once.
+*/
+   i++;
+
+   sub = submodule_from_path(, _oid, ce->name);
+   if (!sub)
+   /* submodule not checked out? */
+   continue;
+
+   if (is_submodule_active(, ce->name)) {
+   strbuf_addf(_wt, "%s/%s", sub_worktree, sub->path);
+   strbuf_addf(_gd, "%s/modules/%s", sub_gitdir, 
sub->name);
+
+   connect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 
0);
+   connect_wt_gitdir_in_nested(sub_wt.buf, sub_gd.buf);
+
+   strbuf_reset(_wt);
+  

[PATCH 1/6] submodule.h: drop declaration of connect_work_tree_and_git_dir

[Stefan Beller]

The function connect_work_tree_and_git_dir is declared in both submodule.h
and dir.h, such that one of them is redundant. As the function is
implemented in dir.c, drop the declaration from submodule.h

Signed-off-by: Stefan Beller 
---
 submodule.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/submodule.h b/submodule.h
index 9589f13127..e5526f6aaa 100644
--- a/submodule.h
+++ b/submodule.h
@@ -105,7 +105,6 @@ extern int push_unpushed_submodules(struct oid_array 
*commits,
const char **refspec, int refspec_nr,
const struct string_list *push_options,
int dry_run);
-extern void connect_work_tree_and_git_dir(const char *work_tree, const char 
*git_dir);
 /*
  * Given a submodule path (as in the index), return the repository
  * path of that submodule in 'buf'. Return -1 on error or when the
-- 
2.17.0.rc1.321.gba9d0f2565-goog

[PATCH 3/6] submodule-config: add repository argument to submodule_from_{name, path}

[Stefan Beller]

This enables submodule_from_{name, path} to handle arbitrary repositories.
All callers just pass in the_repository, a later patch will pass in other
repos.

While at it remove the extern key word from the declarations.

Reviewed-by: Jonathan Tan 
Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c  | 14 +++---
 submodule-config.c   | 14 --
 submodule-config.h   | 10 ++
 submodule.c  | 30 --
 t/helper/test-submodule-config.c |  6 --
 5 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ee020d4749..a921fbbf56 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -454,7 +454,7 @@ static void init_submodule(const char *path, const char 
*prefix,
 
displaypath = get_submodule_displaypath(path, prefix);
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(the_repository, _oid, path);
 
if (!sub)
die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -621,7 +621,7 @@ static void status_submodule(const char *path, const struct 
object_id *ce_oid,
struct rev_info rev;
int diff_files_result;
 
-   if (!submodule_from_path(_oid, path))
+   if (!submodule_from_path(the_repository, _oid, path))
die(_("no submodule mapping found in .gitmodules for path 
'%s'"),
  path);
 
@@ -741,7 +741,7 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
if (argc != 2)
usage(_("git submodule--helper name "));
 
-   sub = submodule_from_path(_oid, argv[1]);
+   sub = submodule_from_path(the_repository, _oid, argv[1]);
 
if (!sub)
die(_("no submodule mapping found in .gitmodules for path 
'%s'"),
@@ -772,7 +772,7 @@ static void sync_submodule(const char *path, const char 
*prefix,
if (!is_submodule_active(the_repository, path))
return;
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(the_repository, _oid, path);
 
if (sub && sub->url) {
if (starts_with_dot_dot_slash(sub->url) ||
@@ -925,7 +925,7 @@ static void deinit_submodule(const char *path, const char 
*prefix,
struct strbuf sb_config = STRBUF_INIT;
char *sub_git_dir = xstrfmt("%s/.git", path);
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(the_repository, _oid, path);
 
if (!sub || !sub->name)
goto cleanup;
@@ -1367,7 +1367,7 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
goto cleanup;
}
 
-   sub = submodule_from_path(_oid, ce->name);
+   sub = submodule_from_path(the_repository, _oid, ce->name);
 
if (suc->recursive_prefix)
displaypath = relative_path(suc->recursive_prefix,
@@ -1650,7 +1650,7 @@ static const char *remote_submodule_branch(const char 
*path)
const char *branch = NULL;
char *key;
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(the_repository, _oid, path);
if (!sub)
return NULL;
 
diff --git a/submodule-config.c b/submodule-config.c
index a3efff1a34..4b7803e6ed 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -619,18 +619,20 @@ static void gitmodules_read_check(struct repository *repo)
repo_read_gitmodules(repo);
 }
 
-const struct submodule *submodule_from_name(const struct object_id 
*treeish_name,
+const struct submodule *submodule_from_name(struct repository *r,
+   const struct object_id 
*treeish_name,
const char *name)
 {
-   gitmodules_read_check(the_repository);
-   return config_from(the_repository->submodule_cache, treeish_name, name, 
lookup_name);
+   gitmodules_read_check(r);
+   return config_from(r->submodule_cache, treeish_name, name, lookup_name);
 }
 
-const struct submodule *submodule_from_path(const struct object_id 
*treeish_name,
+const struct submodule *submodule_from_path(struct repository *r,
+   const struct object_id 
*treeish_name,
const char *path)
 {
-   gitmodules_read_check(the_repository);
-   return config_from(the_repository->submodule_cache, treeish_name, path, 
lookup_path);
+   gitmodules_read_check(r);
+   return config_from(r->submodule_cache, treeish_name, path, lookup_path);
 }
 
 const struct submodule *submodule_from_cache(struct repository *repo,
diff --git a/submodule-config.h b/submodule-config.h
index 6b71a8cd30..43dfe7dec0 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -39,10 +39,12 @@ extern int parse_update_recurse_submodules_arg(const char 
*opt, const 

[PATCH 2/6] submodule-config: allow submodule_free to handle arbitrary repositories

[Stefan Beller]

At some point we may want to rename the function so that it describes what
it actually does as 'submodule_free' doesn't quite describe that this
clears a repository's submodule cache.  But that's beyond the scope of
this series.

While at it remove the extern key word from its declaration.

Signed-off-by: Stefan Beller 
---
 Documentation/technical/api-submodule-config.txt | 2 +-
 builtin/grep.c   | 2 +-
 submodule-config.c   | 6 +++---
 submodule-config.h   | 2 +-
 t/helper/test-submodule-config.c | 2 +-
 unpack-trees.c   | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/technical/api-submodule-config.txt 
b/Documentation/technical/api-submodule-config.txt
index ee907c4a82..fb06089393 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -38,7 +38,7 @@ Data Structures
 Functions
 -
 
-`void submodule_free()`::
+`void submodule_free(struct repository *r)`::
 
Use these to free the internally cached values.
 
diff --git a/builtin/grep.c b/builtin/grep.c
index 789a89133a..8f04cde18e 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -651,7 +651,7 @@ static int grep_objects(struct grep_opt *opt, const struct 
pathspec *pathspec,
 
/* load the gitmodules file for this rev */
if (recurse_submodules) {
-   submodule_free();
+   submodule_free(repo);
gitmodules_config_oid(_obj->oid);
}
if (grep_object(opt, pathspec, real_obj, list->objects[i].name, 
list->objects[i].path,
diff --git a/submodule-config.c b/submodule-config.c
index 602ba8ca8b..a3efff1a34 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -642,8 +642,8 @@ const struct submodule *submodule_from_cache(struct 
repository *repo,
   key, lookup_path);
 }
 
-void submodule_free(void)
+void submodule_free(struct repository *r)
 {
-   if (the_repository->submodule_cache)
-   submodule_cache_clear(the_repository->submodule_cache);
+   if (r->submodule_cache)
+   submodule_cache_clear(r->submodule_cache);
 }
diff --git a/submodule-config.h b/submodule-config.h
index a5503a5d17..6b71a8cd30 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -46,6 +46,6 @@ extern const struct submodule *submodule_from_path(
 extern const struct submodule *submodule_from_cache(struct repository *repo,
const struct object_id 
*treeish_name,
const char *key);
-extern void submodule_free(void);
+void submodule_free(struct repository *r);
 
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c
index f23db3b19a..9971c5e9dd 100644
--- a/t/helper/test-submodule-config.c
+++ b/t/helper/test-submodule-config.c
@@ -64,7 +64,7 @@ int cmd_main(int argc, const char **argv)
arg += 2;
}
 
-   submodule_free();
+   submodule_free(the_repository);
 
return 0;
 }
diff --git a/unpack-trees.c b/unpack-trees.c
index d5685891a5..05e5fa77eb 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -290,7 +290,7 @@ static void load_gitmodules_file(struct index_state *index,
if (!state && ce->ce_flags & CE_WT_REMOVE) {
repo_read_gitmodules(the_repository);
} else if (state && (ce->ce_flags & CE_UPDATE)) {
-   submodule_free();
+   submodule_free(the_repository);
checkout_entry(ce, state, NULL);
repo_read_gitmodules(the_repository);
}
-- 
2.17.0.rc1.321.gba9d0f2565-goog

[PATCHv2 0/6] Moving submodules with nested submodules

[Stefan Beller]

v2:
* addressed memleaks and messy code in patch 5
* removed the extern keyword where applicable
* extended the commit message, stating we want to rename submodule_free
  in the future.
* picked up Jonathans patch and added it as a nice finish of the series.
  I did not see the need or aesthetic desire to put that patch earlier
  in the series.
  
Thanks,
Stefan

v1:

This fixes the bug reported in [1] ("Bug: moving submodules that have submodules
inside them causes a fatal error in git status")

[1] https://public-inbox.org/git/20180306192017.ga5...@riseup.net/

Thanks,
Stefan

Jonathan Tan (1):
  grep: remove "repo" arg from non-supporting funcs

Stefan Beller (5):
  submodule.h: drop declaration of connect_work_tree_and_git_dir
  submodule-config: allow submodule_free to handle arbitrary
repositories
  submodule-config: add repository argument to submodule_from_{name,
path}
  submodule-config: remove submodule_from_cache
  submodule: fixup nested submodules after moving the submodule

 .../technical/api-submodule-config.txt|  2 +-
 builtin/grep.c| 14 ++---
 builtin/mv.c  |  6 +-
 builtin/submodule--helper.c   | 17 +++--
 dir.c | 63 ++-
 dir.h | 12 +++-
 repository.c  |  2 +-
 submodule-config.c| 29 -
 submodule-config.h| 15 +++--
 submodule.c   | 40 ++--
 submodule.h   |  1 -
 t/helper/test-submodule-config.c  |  8 ++-
 t/t7001-mv.sh |  2 +-
 unpack-trees.c|  2 +-
 14 files changed, 137 insertions(+), 76 deletions(-)

-- 
2.17.0.rc1.321.gba9d0f2565-goog

Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Junio C Hamano]

Daniel Jacques  writes:

> A simple grep suggests that the current test suite doesn't seem to have any
> RUNTIME_PREFIX-specific tests. When I've been running the test suites, I've
> been doing it with a "config.mak" file that explicitly enables
> RUNTIME_PREFIX to get the runtime prefix code tested against the standard
> Git testing suites.
>
> From a Git maintainer's perspective, would such a test be a prerequisite
> for landing this patch series, or is this a good candidate for follow-up
> work to improve our testing coverage?

It would be a nice-to-have follow-up, I would say, but as you two
seem to be working well together and it shouldn't be too involved to
have the minimum test that makes sure the version of "git" being
tested thinks things should be where we think they should be, with
something like...

test_expect_success RUNTIME_PREFIX 'runtime-prefix basics' '
(
# maybe others
safe_unset GIT_EXEC_PATH &&
git --exec-path >actual
) &&
# compute the expected value -- we know the first
# element of $PATH is where we find "git", so things
# should be computable relative to that, perhaps?
echo >expect "${PATH%%:*}/..." &&
# then compare
test_cmp expect actual  
'

so I am hoping such a minimum test to be in the series when it
graduate to 'master' and become a part of a release.  

On the other hand, "make a whole test install and try running it"
may actually be easier but that probably can be done using existing
GIT_TEST_INSTALLED framework?  In short, you would probably do

 - make RUNTIME_PREFIX=YesPlease
 - make RUNTIME_PREFIX=YesPlease DESTDIR=...some..where... install
 - GIT_TEST_INSTALLED=...some..where.../bin make test

or something like that.

Re: Null pointer dereference in git-submodule

[Stefan Beller]

On Wed, Mar 28, 2018 at 9:52 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> Subject: [PATCH v2] submodule: check for NULL return of
>> get_submodule_ref_store()
>>
>> Maybe more imperative, telling what we actually want
>> to achieve instead of what we do?
>>
>>submodule: report deleted submodules as not initialized
>>
>>> If we can't find a ref store for a submodule then assume it the latter
>>> is not initialized (or was removed).  Print a status line accordingly
>>> instead of causing a segmentation fault by passing NULL as the first
>>> parameter of refs_head_ref().
>>
>> Thanks for the message here. Looks good!
>> ...
>> Which would be added in t/t7400-submodule-basic.sh
>>
>> Thanks for coming up with a sensible patch!
>
> I take the above to mean that you as a contributor active in this
> area like the general idea in the patch but not volunteering to take
> this topic over

Rereading the discussion, I overlooked the author of the second patch
to be Rene (for some reason I thought someone else would have
written that patch. Sorry about that!)

> and instead trust René to tie the loose ends with a
> reroll, taking hints from your suggestions?

As Rene likes. I can reroll that patch with a test, too.
I'd pick it up after rerolling the series from yesterday
(moving nested submodules).

> I just wanted to make sure that we won't be confused whose turn it
> is next (e.g. me waiting for update to t7400 from you or René doing
> the same).

Thanks,
Stefan

Re: [PATCH 00/10] Hash-independent tests (part 1)

[Junio C Hamano]

Johannes Schindelin  writes:

>> Ideally, the existing one be annotated with prereq SHA1, and also
>> duplicated with a tweak to cause the same kind of (half-)collision
>> under the NewHash and be annotated with prereq NewHash.
>
> That's a good idea. I wonder whether we want to be a bit more specific,
> though, so that we have something grep'able for the future? Something like
> SHA1_SHORT_COLLISION or some such?

Sorry, you lost me.  

What I meant was that a test, for example, that expects the object
name for an empty blob begins with e69de29 is valid ONLY when Git is
using SHA-1 to name objects, so such a test should be run only when
Git is using SHA-1 and no other hash.  All tests in t1512 that
expects the sequence of events in it would yield blobs and trees
whose names have many leading "0"s are the same way.

I think it would do to have a single prerequisite to cover all such
tests: "Run this piece of test only when Git is using SHA-1 hash".
I do not think you need a set of prerequisites to say "Run this with
SHA-1 because we are testing X" where X may be "disambiguation",
"unique-abbrev", "loose-refs", or whatever.

Re: Null pointer dereference in git-submodule

[Junio C Hamano]

Stefan Beller  writes:

>> Subject: [PATCH v2] submodule: check for NULL return of
> get_submodule_ref_store()
>
> Maybe more imperative, telling what we actually want
> to achieve instead of what we do?
>
>submodule: report deleted submodules as not initialized
>
>> If we can't find a ref store for a submodule then assume it the latter
>> is not initialized (or was removed).  Print a status line accordingly
>> instead of causing a segmentation fault by passing NULL as the first
>> parameter of refs_head_ref().
>
> Thanks for the message here. Looks good!
> ...
> Which would be added in t/t7400-submodule-basic.sh
>
> Thanks for coming up with a sensible patch!

I take the above to mean that you as a contributor active in this
area like the general idea in the patch but not volunteering to take
this topic over and instead trust René to tie the loose ends with a
reroll, taking hints from your suggestions?

I just wanted to make sure that we won't be confused whose turn it
is next (e.g. me waiting for update to t7400 from you or René doing
the same).

Thanks.

Re: [PATCH] t1300: document some aesthetic failures of the config editor

[Johannes Schindelin]

Hi Peff,

On Fri, 29 Mar 2013, Jeff King wrote:

> Subject: [PATCH] t1300: document some aesthetic failures of the config editor
>
> [...]
> 
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 3c96fda..213e5a8 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1087,4 +1087,34 @@ test_expect_success 'barf on incomplete string' '
>   grep " line 3 " error
>  '
>  
> +# good section hygiene
> +test_expect_failure 'unsetting the last key in a section removes header' '
> + cat >.git/config <<-\EOF &&
> + [section]
> + # some intervening lines
> + # that should also be dropped
> +
> + key = value
> + EOF
> +
> + >expect &&
> +
> + git config --unset section.key &&
> + test_cmp expect .git/config
> +'

This would have been good on its own. This documents what a user may
expect, and it is a reasonable expectation.

However, Junio, what you suggested in addition *and squashed in before
merging to `master`*, is not a reasonable expectation. If you are asking
*code* to determine that in a config like this, the second line is a
comment belonging to the section, and the first line is not, that is
totally unreasonable:

# some generic comment on the configuration file itself
# a comment specific to this "section" section.
[section]
# some intervening lines
# that should also be dropped

key = value
# please be careful when you update the above variable


Worse: it does *not* demonstrate a known breakage, at least not precisely,
as what you ask here *is not technically possible*. Not even with NLP, at
least if you drive for 100%. It's just not.

And your example is not even complete, as this is a totally valid config:

[core]
; These settings affect many Git operations; be careful
; what you change here

key = value

Obviously, your example gives the impression that `git config --unset
core.key` shoud *delete* that comment (that obviously is intended to
document the section, not the `key` value).

And this is bad, really bad. And this comment does not make it better:

I think we may not attain that ideal without some natural language
processing of the comments. But hey, no reason not to shoot for the
stars. :)

There *is* a reason, a very good reason *not* to shoot for the stars.

Think about it. The `test_expect_failure` function is intended to
demonstrate bugs, and once those bugs are fixed, the _failure should be
turned into _success. And if somebody looks for work, they can look for
test_expect_failure and find tons of micro-projects.

What you did there was to change some valid demonstration of a bug that
can be fixed to something that cannot be fixed. So if an occasional lurker
comes along, sees what you expect to be fixed, they would have said
"Whoa!" and you lost a contribution.

Let's avoid such a "shoot for the stars [... and get nothing, not even an
incremental improvement in return...]" in the future.

On a positive note: I just finished work on a set of patches addressing
this:
https://github.com/git/git/compare/master...dscho:empty-config-section (I
plan on submitting this tomorrow)

Ciao,
Dscho

P.S.: While I am already raising awareness about unintended consequences,
let me also add this: that "cute" feature that unambiguous abbreviations
of options are allowed bit me royally today. Try this: `git config
--remove section.key`. And then be surprised that it does not work, even
if you have that entry. The reason? The option `--remove` is a unique
abbreviation of `--remove-section`, so even if I clearly meant `--unset`,
that feature (that every Git user I tell about it is very surprised to
hear about, so it is not like it is helping a lot of users) has the
unintended consequence of being completely wrong. It would have been
better to tell me that there is no `--remove` option.

Re: Bug: duplicate sections in .git/config after remote removal

[Johannes Schindelin]

Hi Stefan & Jason,

On Tue, 27 Mar 2018, Stefan Beller wrote:

> On Tue, Mar 27, 2018 at 1:41 PM Jason Frey  wrote:
> 
> > at which point you can see the duplicate sections (even though one is
> > empty).  Also note that if you do the steps again, you will be left
> > with 3 sections, 2 of which are empty.  This process can be repeated
> > over and over.
> 
> I agree that this is an issue for the user, and there were some attempts
> to fix it in the past. (feel free to dig them up in the archive at
> https://public-inbox.org/git)

Note: as far as I remember, the attempted fixes were exclusively trying to
remove the empty section. But this report suggests that we could instead
*keep* empty sections, but then reuse them when a new value is added.

> IIRC the problem is (a) with the loose file format (What if the user put
> a valuable comment just after or before the '[branch "master"]' line?)
> as well as (b) the way the parser/writer works (single pass, line by line)
> 
> (b) specifically made it a "huge effort, but little return" bug,
> so nobody got around for a proper fix.

Yes, (a) makes removing an empty section something less of a desirable
thing. Unless there are no comments before and after the section, of
course, and yes, (b) is a real thing.

On a positive note: I just finished work on a set of patches addressing
this:
https://github.com/git/git/compare/master...dscho:empty-config-section (I
plan on submitting this tomorrow)

Ciao,
Dscho

[PATCH v6] json-writer: fixups for V5

[git]

From: Jeff Hostetler 

Please squash this onto the top of jh/json-writer.

Fix leading whitespace in t0019 using tricked suggested by Junio.
Fix unnecessary cast for intmax_t suggested by Wink.

Signed-off-by: Jeff Hostetler 
---
 json-writer.c  |  4 ++--
 t/t0019-json-writer.sh | 36 ++--
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/json-writer.c b/json-writer.c
index 1b49158..dbfcf70 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -165,7 +165,7 @@ void jw_object_string(struct json_writer *jw, const char 
*key, const char *value
 void jw_object_intmax(struct json_writer *jw, const char *key, intmax_t value)
 {
object_common(jw, key);
-   strbuf_addf(>json, "%"PRIdMAX, (intmax_t)value);
+   strbuf_addf(>json, "%"PRIdMAX, value);
 }
 
 void jw_object_double(struct json_writer *jw, const char *key, int precision,
@@ -303,7 +303,7 @@ void jw_array_string(struct json_writer *jw, const char 
*value)
 void jw_array_intmax(struct json_writer *jw, intmax_t value)
 {
array_common(jw);
-   strbuf_addf(>json, "%"PRIdMAX, (intmax_t)value);
+   strbuf_addf(>json, "%"PRIdMAX, value);
 }
 
 void jw_array_double(struct json_writer *jw, int precision, double value)
diff --git a/t/t0019-json-writer.sh b/t/t0019-json-writer.sh
index a04c055..bd6d474 100755
--- a/t/t0019-json-writer.sh
+++ b/t/t0019-json-writer.sh
@@ -166,24 +166,24 @@ test_expect_success 'nested inline object and array 2' '
 '
 
 test_expect_success 'pretty nested inline object and array 2' '
-   cat >expect actual \
--pretty \
@object \
-- 
2.9.3

Re: [PATCH 0/3] shortlog: do not accept revisions when run outside repo

[Martin Ågren]

On 28 March 2018 at 10:48, Jeff King  wrote:
> On Sat, Mar 10, 2018 at 12:52:09PM +0100, Martin Ågren wrote:
>
>> Someone trying this out might notice that `man git-shortlog` renders
>> "\--" as "\--", which is not wanted. (Also visible on git-scm.com...)
>> There is quite some history around such double-slashes and compatibility
>> with AsciiDoc-versions, so I'd rather not do a "while at it" there.
>> Regardless of the destiny of patch 1/3, I will follow up later to
>> address various forms of "\--" throughout the tree.
>
> I didn't see any follow-up here, but in case you were delaying because
> the history search seemed boring: dropping the backslash is the right
> thing to do.  See the discussion in 1c262bb7b2 (doc: convert \--option
> to --option, 2015-05-13).

Thanks for pinging and thanks for the pointer. That commit is indeed
helpful and I am referencing it in a local topic, which I will submit
once ma/shortlog-revparse hits master.

Martin

Re: [RFC] Rebasing merges: a jorney to the ultimate solution(RoadClear)

[Sergey Organov]

Jacob Keller  writes:

> On Tue, Mar 27, 2018 at 10:57 PM, Sergey Organov  wrote:
>>
>> Hi Johannes,
>>
>> Johannes Schindelin  writes:

[...]

> I'm pretty sure the fact has already been accepted, as he did indeed
> implement and develop a strategy for rebasing the merges (Phillip's
> strategy). He hasn't chosen to re-write all the code such that it was
> "always" this method, but rather kept it as an incremental patch on top as
> it makes it easier to review the changes since we've already spent time
> looking at and reviewing the --recreate-merges patches.

That's perfectly OK with me, except that he apparently still can't
accept the fact that rebasing a non-merge is not fundamentally different
from rebasing a merge.

"Rebase non-merge" is just a special case of generic "rebase commit",
provided we do have generic method that is capable to rebase any commit,
and we do have it, Phillip's or not.

> Having watched from the sidelines, I've been unable to completely
> understand and parse the strategies completely, but I've also found
> Phillip's method to be easier to understand.

It doesn't matter at all for this particular discussion. Let's call the
method "rebase a commit", a black-box, that is capable to rebase any
commit. I don't care what implementation is inside. Rebasing a commit is
still rebasing a commit, and it should not be called "merge" in the todo
list.

> As someone who's read the discussion on the sidelines, it certainly
> does feel like there is some misunderstanding on both sides. Neither
> of you have been able to get the other to see what you clearly both
> believe strongly.

Calling "rebase" operation "merge" is wrong no matter what method is
used to rebase a commit. Isn't it obvious? It's currently called "pick"
in the todo and it seems natural to continue to use that name for
picking a commit, whatever number of parents it happens to have.

> Unfortunately I do not have any suggestion as to how to resolve the
> misunderstanding.

This sub-thread is not about method at all, so no resolution on that
matter is required here. This sub-thread is about todo format only.

> Sergey's method appears to me to be more complex, and I agree that the
> extra steps could cause more merge conflicts, at least in how it was
> originally conceptualized and implemented. It is possible that we are
> mis-understanding the terminology for U1 and U2? It sure seems like it
> introduces more changes for merge conflicts than the strategy proposed by
> Phillip. However, the latest editions also sound a lot closer to Phillip's
> strategy in general, so maybe I have mis-understood how it works and what
> is fundamentally different about the two strategies.

There is nothing fundamentally different between them and thus I don't
care in this discussion what exact method is being used.

-- Sergey

Re: [RFC] Rebasing merges: a jorney to the ultimate solution(RoadClear)

[Sergey Organov]

Jacob Keller  writes:

> On Tue, Mar 27, 2018 at 10:57 PM, Sergey Organov  wrote:
>>
>> Hi Johannes,
>>
>> Johannes Schindelin  writes:
>> > Hi Sergey,
>> >
>>
>> [...]
>>
>> >> >> Reusing existing concepts where possible doesn`t have this problem.
>> >> >
>> >> > Existing concepts are great. As long as they fit the requirements of
>> >> > the new scenarios. In this case, `pick` does *not* fit the
> requirement
>> >> > of "rebase a merge commit".
>> >>
>> >> It does, provided you use suitable syntax.
>> >
>> > You know what `pick` would also do, provided you use suitable syntax?
> Pick
>> > your nose.
>> >
>> > Don't blame me for this ridiculous turn the discussion took.
>> >
>> > Of course, using the suitable syntax you can do anything. Unless there
> is
>> > *already* a syntax and you cannot break it for backwards-compatibility
>> > reasons, as is the case here.
>>
>> Backward compatibility to what? To a broken '--preserve-merges'? I had a
>> feel you've invented '--recreate-merges' exactly to break that
>> compatibility. No?
>>
>> Or is it "Backwards compatibility of a feature that existed only as a
>> topic branch in `next` before being worked on more?", as you say
>> yourself below?
>>
>
> I'm pretty sure he meant that changing the meaning and behavior of "pick"
> is incompatible, as people use scripts which check the edit lists, and
> these scripts would expect pick to behave in a certain way.

Are we still speaking about that new --recreate-merges feature? You
already care for compatibility for it? You expect there are already
scripts that use it?

Once again, it seems like you care and don't care about backward
compatibility at the same time, here is your phrase below:

"He absolutely cares about compatibility, but in this case, the feature
has not yet been merged into an official release."

Are we still speaking about that new --recreate-merges feature?

Do you guys care for compatibility for this particular --recreate-merges
feature or not? I'm lost. "Yes" or "No" answer, if you please!

-- Sergey

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Duy Nguyen]

On Wed, Mar 28, 2018 at 11:52 AM, Jeff King  wrote:
> On Tue, Mar 27, 2018 at 07:30:24PM +0200, Duy Nguyen wrote:
>
>> On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote:
>> > I would rather have something like ref_store_reinit() in the same
>> > spirit as the second call of set_git_dir() in setup_work_tree. It is
>> > hacky, but it works and keeps changes to minimal (so that it could be
>> > easily replaced later).
>>
>> So in the name of hacky and dirty things, it would look something like
>> this. This passed your test case. The test suite is still running
>> (slow laptop) but I don't expect breakages there.
>
> I think this is the right direction. I mentioned in my last reply that
> it would be nice for this to be a bit more generic, in case we need to
> use it again (and also just to keep the module boundaries sane).

Yes, that's why I called it hacky and dirty :) I keep thinking about
this, so I will probably fix it in a nicer way.

> This part confused me at first:
>
>> +void make_main_ref_store_use_absolute_paths(void)
>> +{
>> + files_force_absolute_paths(get_main_ref_store());
>> +}
>> +
>> +void make_main_ref_store_use_relative_paths(const char *cwd)
>> +{
>> + files_make_relative_paths(get_main_ref_store(), cwd);
>> +}
>
> since I thought you were actually turning things into absolute paths.
> But your procedure is basically "turn absolute, then after chdir, turn
> them back relative".
>
> I think it might be clearer if a single call is given both the old and
> new paths. That requires the caller of chdir() storing getcwd() before
> it moves, but I don't think that should be a big deal.

The problem is switching relative paths relies on the old $CWD if I'm
not mistaken and we need  getcwd() for this. I'd love to have one
callback that says "$CWD has been switched from this path to that
path, do whatever you need to" that can be called any time, before or
after chdir(). I'll look more into it.
-- 
Duy

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King]

On Tue, Mar 27, 2018 at 07:30:24PM +0200, Duy Nguyen wrote:

> On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote:
> > I would rather have something like ref_store_reinit() in the same
> > spirit as the second call of set_git_dir() in setup_work_tree. It is
> > hacky, but it works and keeps changes to minimal (so that it could be
> > easily replaced later).
> 
> So in the name of hacky and dirty things, it would look something like
> this. This passed your test case. The test suite is still running
> (slow laptop) but I don't expect breakages there.

I think this is the right direction. I mentioned in my last reply that
it would be nice for this to be a bit more generic, in case we need to
use it again (and also just to keep the module boundaries sane).

This part confused me at first:

> +void make_main_ref_store_use_absolute_paths(void)
> +{
> + files_force_absolute_paths(get_main_ref_store());
> +}
> +
> +void make_main_ref_store_use_relative_paths(const char *cwd)
> +{
> + files_make_relative_paths(get_main_ref_store(), cwd);
> +}

since I thought you were actually turning things into absolute paths.
But your procedure is basically "turn absolute, then after chdir, turn
them back relative".

I think it might be clearer if a single call is given both the old and
new paths. That requires the caller of chdir() storing getcwd() before
it moves, but I don't think that should be a big deal.

-Peff

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King]

On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote:

> > I don't quite get why f57f37e2 doesn't want to call git_path(). Is it to
> > avoid the way the path is munged? Or is it to avoid some lazy-setup that
> > is triggered by calling get_git_dir() at all (which doesn't make much
> > sense to me, because we'd already have called get_git_dir() much
> > earlier). Or is it just because we may sometimes fill in refs->git_dir
> > with something besides get_git_dir() (for a submodule or worktree or
> > something)?
> 
> None of those, I think. git_path() does some magic to translate paths
> so that refs/... ends up with $GIT_COMMON_DIR/refs/... while "index"
> ends up with $GIT_DIR/index. Michael wanted to avoid that magic and
> keep the control within refs code (i.e. this code knows refs/ and
> packed-refs are shared, and pseudo refs are not, what git_path()
> decides does not matter).

Ah, OK (that is my first one, "avoid the way the path is munged", but
obviously I didn't spell it out very clearly).

> > Hmm. Typing that out, it seems like (3) is probably the right path.
> > Something like the patch below seems to fix it and passes the tests.
> 
> Honestly I think this is just another way to work around the problem
> (with even more changes than your abspath approach). The problem is
> with setup_work_tree(). We create a ref store at a specific location
> and it should stay working without lazily calling get_git_dir(), which
> has nothing to do (anymore) with the path we have given a ref store.
> If somebody changes a global setting like $CWD, it should be well
> communicated to everybody involved.

Yeah, I agree that the root of the problem is not the caching of
get_git_dir(), but that chdir() may invalidate assumptions made by other
parts of the program.

> I would rather have something like ref_store_reinit() in the same
> spirit as the second call of set_git_dir() in setup_work_tree. It is
> hacky, but it works and keeps changes to minimal (so that it could be
> easily replaced later).

So the non-hacky solution is to inform all callers that we've changed
directories, and they may need to recompute any relative paths.

It does seem backwards for setup_work_tree() to need to know about the
refs code. Should we have a system by which interested code can register
to learn about changes to global state? E.g., something like:

  typedef void (*chdir_notify_cb)(const char *old_cwd,
  const char *new_cwd,
  void *data);

  /* Register interest in hearing about chdir */
  void chdir_notify_register(chdir_notify_cb cb, void *data);

  /* Do a chdir and then tell everybody about it */
  void chdir_notify(const char *path);

Then the ref code (or anybody else) should be able to write a function
to normalize a relative path from the old_cwd into a relative one from
the new_cwd.

-Peff

Re: [PATCH 0/3] shortlog: do not accept revisions when run outside repo

[Jeff King]

On Sat, Mar 10, 2018 at 12:52:09PM +0100, Martin Ågren wrote:

> Someone trying this out might notice that `man git-shortlog` renders
> "\--" as "\--", which is not wanted. (Also visible on git-scm.com...)
> There is quite some history around such double-slashes and compatibility
> with AsciiDoc-versions, so I'd rather not do a "while at it" there.
> Regardless of the destiny of patch 1/3, I will follow up later to
> address various forms of "\--" throughout the tree.

I didn't see any follow-up here, but in case you were delaying because
the history search seemed boring: dropping the backslash is the right
thing to do.  See the discussion in 1c262bb7b2 (doc: convert \--option
to --option, 2015-05-13).

-Peff

RSVP

[omann7788]

Contact for T & C.

Re: Bug: duplicate sections in .git/config after remote removal

[Philip Oakley]

From: "Ævar Arnfjörð Bjarmason" 

On Tue, Mar 27 2018, Jason Frey wrote:


While the impact of this bug is minimal, and git itself is not
affected, it can affect external tools that want to read the
.git/config file, expecting unique section names.

To reproduce:

Given the following example .git/config file (I am leaving out the
[core] section for brevity):

[remote "origin"]
url = g...@github.com:Fryguy/example.git
fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
remote = origin
merge = refs/heads/master

Running `git remote rm origin` will result in the following contents:

[branch "master"]

Running `git remote add origin g...@github.com:Fryguy/example.git` will
result in the following contents:

[branch "master"]
[remote "origin"]
url = g...@github.com:Fryguy/example.git
fetch = +refs/heads/*:refs/remotes/origin/*

And finally, running `git fetch origin; git branch -u origin/master`
will result in the following contents:

[branch "master"]
[remote "origin"]
url = g...@github.com:Fryguy/example.git
fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
remote = origin
merge = refs/heads/master

at which point you can see the duplicate sections (even though one is
empty).  Also note that if you do the steps again, you will be left
with 3 sections, 2 of which are empty.  This process can be repeated
over and over.


This can be annoying and result in some very verbose config files when
we automatically edit them, e.g.:

   (rm -v /tmp/test.ini; for i in {1..3}; do git config -f /tmp/test.ini 
foo.bar 0 && git config -f /tmp/test.ini --unset foo.bar; done; cat 
/tmp/test.ini)

   removed '/tmp/test.ini'
   [foo]
   [foo]
   [foo]

But it's not so clear that it should be called a bug, yes we could be a
bit smarter and not add obvious crap like the example above (duplicate
sections at the end), but it gets less obvious in more complex cases,
see my c8b2cec09e ("branch: add test for -m renaming multiple config
sections", 2017-06-18) for one such example.

Git has a config format that's hybrid human/machine editable. Consider a
case like:

   [gc]
   ;; Here's all the gc config we set up to avoid the great outage of 2015
   autoDetach = false
   ;; Our aliases
   [alias]
   st = status

Now, if I run `git config gc.auto 0` is it better if we end up with:

   [gc]
   ;; Here's all the gc config we set up to avoid the great outage of 2015
   autoDetach = false
   auto = 0
   ;; Our aliases
   [alias]
   st = status

Or something that makes it more clear that a machine added something at
the end:

   [gc]
   ;; Here's all the gc config we set up to avoid the great outage of 2015
   autoDetach = false
   ;; Our aliases
   [alias]
   st = status
   [gc]
   auto = 0

Most importantly though, regardless of what we decide to do when we
machine-edit the file, it's also human-editable, and being able to
repeat sections is part of our config format that you're simply going to
have to deal with.


One option may be to create  a simple 'lint' style checker that simply 
hiughlights and suggests options so the user can decide for themselves what 
they need to do. This would help span the gap between hard format and the 
soft format capabiulities of machine readable ini files, the Git config 
reader and being human readable.


Thus duplicate sections would be noted, likewise the presence of comments 
immediately preceding a section header, or terminating a section (with or 
without spacing?), etc.Such a config_lint could reside in the contrib as a 
supprt tool, and may in the long term be a guide to a common format. 
However, as noted, it would be more of a long term aspiration..





The external tool (presumably some generic *.ini parser) you're trying
to point at git's config is broken for that purpose if it doesn't handle
duplicate sections. You're probably better off trying to parse `git
config --list --null` than trying to make it work.

I don't think we'd ever want to get rid of this feature, it's *very*
useful. Both for config via the include macro, and for people to
manually paste some config they want to try out to the end of their
config, without having to manually edit it to incorporate it into their
already existing sections.



--
Philip