Re: [PATCH 2/3] stripspace: respect repository config

[Jeff King]

On Tue, Nov 22, 2016 at 01:55:07PM -0800, Junio C Hamano wrote:

> > And this test makes sense. Even without "sub", it would show the
> > regression, but it's a good idea to test the sub-directory case to cover
> > the path-munging.
> 
> Yup.  Obviously during my initial attempt I was scratching my head
> wondering where these two files went--they were later found inside
> t/ directory which was really bad ;-)

Heh. Yeah, it's nice when the failure mode doesn't escape the trash
directory, but it's probably not worth worrying about too much.

> > In the "archive --remote" test I added, we may want to do the same to
> > show that "--output" points at the correct path.
> 
> Perhaps something like this.  By going down one level, we make sure
> that it is not sufficient to accidentally read from .git/config to
> find out what 'foo' is, and also ../b5-nick.tar that is relative to
> the prefix (aka 'a/') ends up at the top-level.

Yeah, your modification looks good.

-Peff

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

Jeff King  writes:

>> +test_expect_success 'mailinfo with mailinfo.scissors config' '
>> +test_config mailinfo.scissors true &&
>> +(
>> +mkdir sub &&
>> +cd sub &&
>> +git mailinfo ../msg0014.sc ../patch0014.sc <../0014 
>> >../info0014.sc
>> +) &&
>> +test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
>> +test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
>> +test_cmp "$DATA/info0014--scissors" info0014.sc
>> +'
>
> And this test makes sense. Even without "sub", it would show the
> regression, but it's a good idea to test the sub-directory case to cover
> the path-munging.

Yup.  Obviously during my initial attempt I was scratching my head
wondering where these two files went--they were later found inside
t/ directory which was really bad ;-)

> In the "archive --remote" test I added, we may want to do the same to
> show that "--output" points at the correct path.

Perhaps something like this.  By going down one level, we make sure
that it is not sufficient to accidentally read from .git/config to
find out what 'foo' is, and also ../b5-nick.tar that is relative to
the prefix (aka 'a/') ends up at the top-level.

 t/t5000-tar-tree.sh | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 09df7f0458..830bf2a2f6 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -195,7 +195,10 @@ test_expect_success 'git archive --remote' \
 
 test_expect_success 'git archive --remote with configured remote' '
git config remote.foo.url . &&
-   git archive --remote=foo HEAD >b5-nick.tar &&
+   (
+   cd a &&
+   git archive --remote=foo --output=../b5-nick.tar HEAD
+   ) &&
test_cmp_bin b.tar b5-nick.tar
 '
 

Re: [PATCH 2/3] stripspace: respect repository config

[Jeff King]

On Tue, Nov 22, 2016 at 01:22:24PM -0800, Junio C Hamano wrote:

> OK.  The "mailinfo" part turns out a bit more than RUN_SETUP_GENTLY
> as it takes paths from the command line that needs to be adjusted
> for the prefix.

I wondered at first if our lack of parse-options here was holding us
back from using OPT_FILENAME. Though even if that was the case, it is
probably better to do the minimal fix in this instance, rather than
converting the option parsing.

However, the paths which need munged are not even options, they are
arguments. So we wouldn't be able to rely on OPT_FILENAME anyway. I'm
surprised we haven't had to deal with this elsewhere, but I guess most
commands don't take arbitrary filenames (things like `add` take
pathspecs, and then the pathspec machinery takes care of the details).

The only other case I could think of is git-apply, and it does use
prefix_filename() correctly.

> +static char *prefix_copy(const char *prefix, const char *filename)
> +{
> + if (!prefix || is_absolute_path(filename))
> + return xstrdup(filename);
> + return xstrdup(prefix_filename(prefix, strlen(prefix), filename));
> +}

So this is more or less a copy of parse-option's fix_filename(), which
makes sense.

I noticed that git-apply does not check is_absolute_path(), but that's
OK, as prefix_filename() does. So I think it would be correct to drop it
here, but it doesn't matter much either way.

>  int cmd_mailinfo(int argc, const char **argv, const char *prefix)
>  {
>   const char *def_charset;
>   struct mailinfo mi;
>   int status;
> + char *msgfile, *patchfile;
>  
> - /* NEEDSWORK: might want to do the optional .git/ directory
> -  * discovery
> -  */
>   setup_mailinfo();

This part looks straightforward and correct. Dropping the NEEDSWORK is a
nice bonus.

> +test_expect_success 'mailinfo with mailinfo.scissors config' '
> + test_config mailinfo.scissors true &&
> + (
> + mkdir sub &&
> + cd sub &&
> + git mailinfo ../msg0014.sc ../patch0014.sc <../0014 
> >../info0014.sc
> + ) &&
> + test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
> + test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
> + test_cmp "$DATA/info0014--scissors" info0014.sc
> +'

And this test makes sense. Even without "sub", it would show the
regression, but it's a good idea to test the sub-directory case to cover
the path-munging.

In the "archive --remote" test I added, we may want to do the same to
show that "--output" points at the correct path.

-Peff

Re: [PATCH 2/3] stripspace: respect repository config

[Jeff King]

On Tue, Nov 22, 2016 at 04:19:20PM -0500, Jeff King wrote:

> > > Do you want to do another round of -rc3? Ship with the
> > > minor regressions and fix them up in v2.11.1?
> > 
> > I am leaning towards the former (though we may also end up doing the
> > latter).
> 
> I think I'd lead towards -rc3, as well. Our schedule is somewhat

I meant s/lead/lean/, of course.

> Something like this, which does all but the last (and that should
> probably happen separately post-release).

As usual, I'm utterly confused by the $(pwd) versus $PWD thing. So let
me review what I did to see if it makes sense. :)

> +# run "$@" inside a non-git directory
> +nongit () {
> + test -d non-repo ||
> + mkdir non-repo ||
> + return 1
> +
> + (
> + GIT_CEILING_DIRECTORIES=$(pwd) &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd non-repo &&
> + "$@"
> + )
> +}

I copied this bit from t1515, which sets up a similar scenario. I think
it _probably_ works either way because of the environment-variable
conversion magic that Windows bash does.

> +test_expect_success 'git archive --remote outside of a git repo' '
> + git archive HEAD >expect.tar &&
> + nongit git archive --remote="$PWD" HEAD >actual.tar &&
> + test_cmp_bin expect.tar actual.tar
> +'

I'm not sure if it matters here. I almost wrote $TRASH_DIRECTORY, but
that I think is in the same form as $PWD. Either would be fine.

-Peff

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

Jeff King  writes:

> On Tue, Nov 22, 2016 at 12:24:15PM -0800, Junio C Hamano wrote:
>
>> > Do you want to do another round of -rc3? Ship with the
>> > minor regressions and fix them up in v2.11.1?
>> 
>> I am leaning towards the former (though we may also end up doing the
>> latter).
>
> I think I'd lead towards -rc3, as well. Our schedule is somewhat
> arbitrary anyway, and one week is less important than somebody picking
> up the new release and trying to puzzle out a regression that we already
> have a fix for.

OK.  The "mailinfo" part turns out a bit more than RUN_SETUP_GENTLY
as it takes paths from the command line that needs to be adjusted
for the prefix.

-- >8 --
From: Junio C Hamano 
Date: Tue, 22 Nov 2016 13:13:16 -0800
Subject: [PATCH] mailinfo: read local configuration

Since b9605bc4f2 ("config: only read .git/config from configured
repos", 2016-09-12), we do not read from ".git/config" unless we
know we are in a repository.  "git mailinfo" however didn't do the
repository discovery and instead relied on the old behaviour.  This
was mostly OK because it was merely run as a helper program by other
porcelain scripts that first chdir's up to the root of the working
tree.

Teach the command to run a "gentle" version of repository discovery
so that local configuration variables like mailinfo.scissors are
honoured.

Signed-off-by: Junio C Hamano 
---
 builtin/mailinfo.c  | 19 +++
 git.c   |  2 +-
 t/t5100-mailinfo.sh | 13 +
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index f6df274111..e3b62f2fc7 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -11,15 +11,20 @@
 static const char mailinfo_usage[] =
"git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding= 
| -n] [--scissors | --no-scissors]   < mail >info";
 
+static char *prefix_copy(const char *prefix, const char *filename)
+{
+   if (!prefix || is_absolute_path(filename))
+   return xstrdup(filename);
+   return xstrdup(prefix_filename(prefix, strlen(prefix), filename));
+}
+
 int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 {
const char *def_charset;
struct mailinfo mi;
int status;
+   char *msgfile, *patchfile;
 
-   /* NEEDSWORK: might want to do the optional .git/ directory
-* discovery
-*/
setup_mailinfo();
 
def_charset = get_commit_output_encoding();
@@ -54,8 +59,14 @@ int cmd_mailinfo(int argc, const char **argv, const char 
*prefix)
 
mi.input = stdin;
mi.output = stdout;
-   status = !!mailinfo(, argv[1], argv[2]);
+
+   msgfile = prefix_copy(prefix, argv[1]);
+   patchfile = prefix_copy(prefix, argv[2]);
+
+   status = !!mailinfo(, msgfile, patchfile);
clear_mailinfo();
 
+   free(msgfile);
+   free(patchfile);
return status;
 }
diff --git a/git.c b/git.c
index efa1059fe0..fd3abf85d2 100644
--- a/git.c
+++ b/git.c
@@ -445,7 +445,7 @@ static struct cmd_struct commands[] = {
{ "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },
{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
{ "ls-tree", cmd_ls_tree, RUN_SETUP },
-   { "mailinfo", cmd_mailinfo },
+   { "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY },
{ "mailsplit", cmd_mailsplit },
{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
{ "merge-base", cmd_merge_base, RUN_SETUP },
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index e6b995161e..7171f67539 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -158,4 +158,17 @@ test_expect_success 'mailinfo handles rfc2822 comment' '
test_cmp "$DATA/comment.expect" comment/info
 '
 
+test_expect_success 'mailinfo with mailinfo.scissors config' '
+   test_config mailinfo.scissors true &&
+   (
+   mkdir sub &&
+   cd sub &&
+   git mailinfo ../msg0014.sc ../patch0014.sc <../0014 
>../info0014.sc
+   ) &&
+   test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
+   test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
+   test_cmp "$DATA/info0014--scissors" info0014.sc
+'
+
+
 test_done
-- 
2.11.0-rc2-170-g4c384f318f

Re: [PATCH 2/3] stripspace: respect repository config

[Jeff King]

On Tue, Nov 22, 2016 at 12:24:15PM -0800, Junio C Hamano wrote:

> > Do you want to do another round of -rc3? Ship with the
> > minor regressions and fix them up in v2.11.1?
> 
> I am leaning towards the former (though we may also end up doing the
> latter).

I think I'd lead towards -rc3, as well. Our schedule is somewhat
arbitrary anyway, and one week is less important than somebody picking
up the new release and trying to puzzle out a regression that we already
have a fix for.

> Here is an initial attempt.  It turns out that write_archive() must
> be callable outside a repository to respond to "git archive --list",
> which happens in parse_archive_args(), and the "have-repository?"
> check cannot be moved before that to the caller.

Right, that makes sense. t5000 already tests git-archive outside of a
repo, although it does it in a funny way (it sets GIT_DIR rather than
relying on GIT_CEILING_DIRECTORIES). We should cover that and make sure
that --remote works outside of a repo. Those work now, but we want to
make sure we don't regress them.

And then we want to test that --remote can read a configured remote
name.

And then post-v2.11, we'd want to check that --remote=foo outside of a
repo produces a sane error instead of looking for a bogus
$GIT_DIR/remotes/foo.

Something like this, which does all but the last (and that should
probably happen separately post-release).

---
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 80b238734..09df7f045 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -94,6 +94,20 @@ check_tar() {
'
 }
 
+# run "$@" inside a non-git directory
+nongit () {
+   test -d non-repo ||
+   mkdir non-repo ||
+   return 1
+
+   (
+   GIT_CEILING_DIRECTORIES=$(pwd) &&
+   export GIT_CEILING_DIRECTORIES &&
+   cd non-repo &&
+   "$@"
+   )
+}
+
 test_expect_success \
 'populate workdir' \
 'mkdir a &&
@@ -179,6 +193,12 @@ test_expect_success 'git archive --remote' \
 'git archive --remote=. HEAD >b5.tar &&
 test_cmp_bin b.tar b5.tar'
 
+test_expect_success 'git archive --remote with configured remote' '
+   git config remote.foo.url . &&
+   git archive --remote=foo HEAD >b5-nick.tar &&
+   test_cmp_bin b.tar b5-nick.tar
+'
+
 test_expect_success \
 'validate file modification time' \
 'mkdir extract &&
@@ -197,9 +217,15 @@ test_expect_success 'git archive with --output, override 
inferred format' '
test_cmp_bin b.tar d4.zip
 '
 
-test_expect_success \
-'git archive --list outside of a git repo' \
-'GIT_DIR=some/non-existing/directory git archive --list'
+test_expect_success 'git archive --list outside of a git repo' '
+   nongit git archive --list
+'
+
+test_expect_success 'git archive --remote outside of a git repo' '
+   git archive HEAD >expect.tar &&
+   nongit git archive --remote="$PWD" HEAD >actual.tar &&
+   test_cmp_bin expect.tar actual.tar
+'
 
 test_expect_success 'clients cannot access unreachable commits' '
test_commit unreachable &&

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

Jeff King  writes:

> So what do you want to do for v2.11? I think there are minor regressions
> in stripspace, archive, and mailinfo with respect to reading
> .git/config. The right solution in each case is to do a gentle repo
> setup. We have patches for stripspace already, but not yet for the other
> two. They _should_ be fairly trivial, but we are getting awfully close
> to the release. 

Right.

> Do you want to do another round of -rc3? Ship with the
> minor regressions and fix them up in v2.11.1?

I am leaning towards the former (though we may also end up doing the
latter).

Here is an initial attempt.  It turns out that write_archive() must
be callable outside a repository to respond to "git archive --list",
which happens in parse_archive_args(), and the "have-repository?"
check cannot be moved before that to the caller.

No tests yet.  Help appreciated ;-)


 archive.c| 9 ++---
 archive.h| 2 +-
 builtin/archive.c| 6 +++---
 builtin/upload-archive.c | 2 +-
 git.c| 4 ++--
 5 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/archive.c b/archive.c
index dde1ab4c79..901b10264f 100644
--- a/archive.c
+++ b/archive.c
@@ -504,15 +504,11 @@ static int parse_archive_args(int argc, const char **argv,
 }
 
 int write_archive(int argc, const char **argv, const char *prefix,
- int setup_prefix, const char *name_hint, int remote)
+ const char *name_hint, int remote)
 {
-   int nongit = 0;
const struct archiver *ar = NULL;
struct archiver_args args;
 
-   if (setup_prefix && prefix == NULL)
-   prefix = setup_git_directory_gently();
-
git_config_get_bool("uploadarchive.allowunreachable", 
_allow_unreachable);
git_config(git_default_config, NULL);
 
@@ -520,7 +516,7 @@ int write_archive(int argc, const char **argv, const char 
*prefix,
init_zip_archiver();
 
argc = parse_archive_args(argc, argv, , , name_hint, remote);
-   if (nongit) {
+   if (!startup_info->have_repository) {
/*
 * We know this will die() with an error, so we could just
 * die ourselves; but its error message will be more specific
@@ -528,7 +524,6 @@ int write_archive(int argc, const char **argv, const char 
*prefix,
 */
setup_git_directory();
}
-
parse_treeish_arg(argv, , prefix, remote);
parse_pathspec_arg(argv + 1, );
 
diff --git a/archive.h b/archive.h
index 4a791e1fed..415e0152e2 100644
--- a/archive.h
+++ b/archive.h
@@ -36,7 +36,7 @@ typedef int (*write_archive_entry_fn_t)(struct archiver_args 
*args,
unsigned int mode);
 
 extern int write_archive_entries(struct archiver_args *args, 
write_archive_entry_fn_t write_entry);
-extern int write_archive(int argc, const char **argv, const char *prefix, int 
setup_prefix, const char *name_hint, int remote);
+extern int write_archive(int argc, const char **argv, const char *prefix, 
const char *name_hint, int remote);
 
 const char *archive_format_from_filename(const char *filename);
 extern void *sha1_file_to_archive(const struct archiver_args *args,
diff --git a/builtin/archive.c b/builtin/archive.c
index 49f491413a..f863465a0f 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -85,8 +85,8 @@ int cmd_archive(int argc, const char **argv, const char 
*prefix)
const char *output = NULL;
const char *remote = NULL;
struct option local_opts[] = {
-   OPT_STRING('o', "output", , N_("file"),
-   N_("write the archive to this file")),
+   OPT_FILENAME('o', "output", ,
+N_("write the archive to this file")),
OPT_STRING(0, "remote", , N_("repo"),
N_("retrieve the archive from remote repository 
")),
OPT_STRING(0, "exec", , N_("command"),
@@ -105,5 +105,5 @@ int cmd_archive(int argc, const char **argv, const char 
*prefix)
 
setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
-   return write_archive(argc, argv, prefix, 1, output, 0);
+   return write_archive(argc, argv, prefix, output, 0);
 }
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index dc872f6a94..cde06977b7 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -43,7 +43,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, 
const char *prefix)
}
 
/* parse all options sent by the client */
-   return write_archive(sent_argv.argc, sent_argv.argv, prefix, 0, NULL, 
1);
+   return write_archive(sent_argv.argc, sent_argv.argv, prefix, NULL, 1);
 }
 
 __attribute__((format (printf, 1, 2)))
diff --git a/git.c b/git.c
index efa1059fe0..e8b2baf2d1 100644
--- a/git.c
+++ b/git.c
@@ -396,7 +396,7 @@ static struct cmd_struct commands[] = {
{ "am", cmd_am, RUN_SETUP | 

Re: [PATCH 2/3] stripspace: respect repository config

[Jeff King]

On Tue, Nov 22, 2016 at 11:10:25AM -0800, Junio C Hamano wrote:

> Archive & Upload-archive:
> 
> "cd Documentation && git archive --remote=origin" immediately hits
> "BUG: setup_git_env called without repository" if your Git is built
> with b1ef400eec ("setup_git_env: avoid blind fall-back to ".git"",
> 2016-10-20), which will not be part of the upcoming release.  And
> 'origin' will probably not be understood from the local config.

Yeah, I think "cd Documentation && git archive --remote" has always been
broken. It doesn't barf until b1ef400eec, but yes, v2.11-rc2 breaks
finding "origin" in the local config.

I agree that doing the "gently" setup will fix the config problem, but
there's more to do to make it work with b1ef400eec outside of a repo.
It looks like read_remotes_file() and read_branches_file() in remote.c
need to learn to silently return when we are not in a git repository (or
alternatively, remote_get_1() should learn not to call them in such a
case).

I doubt this is a critical case (it only kicks in if you are outside a
repo _and_ your name looks like a remote name and not a URL, _and_ it is
not defined by your config, and the result is that we die("BUG") rather
than reporting "you don't have such a remote"). But any time it is
possible to hit a die("BUG"), we should be fixing it.

> I think we can do the "gently" thing there, as we may be retrieving
> a remote archive outside a local repository.  We'd need to tweak
> "output" with prefix to compensate for the case in which the command
> is run from a subdirectory, and probably we need to futz with the
> setup_prefix parameter to write_archive(), as a local caller now
> will know if we are in nongit situation.

Makes sense. I think OPT_FILENAME() would handle "output", but yeah, it
looks like write_archive() already tries to be clever about the setup.

> On the upload-archive side that serves "--remote" request, there is
> enter_repo() so we should be covered OK.

Right, that has to run in a repo, and enter_repo() should handle it.

> Mailinfo:
> 
> We may want a "gently" thing there to pick up local configuration.
> i18n.commitencoding and mailinfo.scissors in local repository would
> be ignored otherwise.

Yeah, agreed.

> Verify-pack:
> 
> This calls git_config() but these days farms out its operation to
> "index-pack", so we should be OK.  We may even want to lose the call
> to git_config() which does not affect anything.

I'd be slightly worried that there is a core.* config option that
affects us in a subtle way (e.g., some portability flag for how we do
run_command()). I don't think there is one now, but it seems like a
potential maintenance pitfall. IOW, I think I'd generally prefer to err
on the side of having everything do a gentle setup and load at least the
default config, to avoid surprises.

I don't think it's a high priority, though.

So what do you want to do for v2.11? I think there are minor regressions
in stripspace, archive, and mailinfo with respect to reading
.git/config. The right solution in each case is to do a gentle repo
setup. We have patches for stripspace already, but not yet for the other
two. They _should_ be fairly trivial, but we are getting awfully close
to the release. Do you want to do another round of -rc3? Ship with the
minor regressions and fix them up in v2.11.1? Last-minute revert the
original config topic that introduced the regressions (I'm biased
against that as the author, but I also think it's dangerous; it's a big
enough topic that it might introduce new problems)?

-Peff

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

Junio C Hamano  writes:

> I think we want to audit the ones without RUN_SETUP* in the command
> table in order to hunt for regression aka "a fix revealed a bug that
> was covered by .git/config accidentally getting read when run from
> the top-level of the working tree", though. We may find unreported
> breakages that we may have to fix.

So I took a brief look at the "PROGRAM_OBJS += ..." in the Makefile
for non-builtin commands, and commands[] array in git.c for builtin
commands to see how bad it looks.


Archive & Upload-archive:

"cd Documentation && git archive --remote=origin" immediately hits
"BUG: setup_git_env called without repository" if your Git is built
with b1ef400eec ("setup_git_env: avoid blind fall-back to ".git"",
2016-10-20), which will not be part of the upcoming release.  And
'origin' will probably not be understood from the local config.

I think we can do the "gently" thing there, as we may be retrieving
a remote archive outside a local repository.  We'd need to tweak
"output" with prefix to compensate for the case in which the command
is run from a subdirectory, and probably we need to futz with the
setup_prefix parameter to write_archive(), as a local caller now
will know if we are in nongit situation.

On the upload-archive side that serves "--remote" request, there is
enter_repo() so we should be covered OK.


Mailinfo:

We may want a "gently" thing there to pick up local configuration.
i18n.commitencoding and mailinfo.scissors in local repository would
be ignored otherwise.


Splitspace:

Dscho fixed this one.


Verify-pack:

This calls git_config() but these days farms out its operation to
"index-pack", so we should be OK.  We may even want to lose the call
to git_config() which does not affect anything.

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

Johannes Schindelin  writes:

>> This conditional config file reading is a trap for similar bugs to
>> happen again. Is there any reason we should not just mark the command
>> RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?
>
> As I plan to slip these patches into Git for Windows v2.11.0, i.e. making
> this a last-minute hot fix, I want to err on the side of caution.

So do I.  As a hot-fix, I'd prefer the patch I queued yesterday.

I think we want to audit the ones without RUN_SETUP* in the command
table in order to hunt for regression aka "a fix revealed a bug that
was covered by .git/config accidentally getting read when run from
the top-level of the working tree", though. We may find unreported
breakages that we may have to fix.

Re: [PATCH 2/3] stripspace: respect repository config

[Johannes Schindelin]

Hi Duy,

On Tue, 22 Nov 2016, Duy Nguyen wrote:

> On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
>  wrote:
> > When eff80a9 (Allow custom "comment char", 2013-01-16) taught the
> > `stripspace` command to respect the config setting `core.commentChar`,
> > it forgot that this variable may be defined in .git/config.
> >
> > So when rebasing interactively with a commentChar defined in the current
> > repository's config, the help text at the bottom of the edit script
> > potentially used an incorrect comment character. This was not only
> > funny-looking, but also resulted in tons of warnings like this one:
> >
> > Warning: the command isn't recognized in the following line
> >  - #
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  builtin/stripspace.c  | 4 +++-
> >  t/t0030-stripspace.sh | 2 +-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> > index 15e716e..1e62a00 100644
> > --- a/builtin/stripspace.c
> > +++ b/builtin/stripspace.c
> > @@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const 
> > char *prefix)
> > if (argc)
> > usage_with_options(stripspace_usage, options);
> >
> > -   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
> > +   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> > +   setup_git_directory_gently(NULL);
> > git_config(git_default_config, NULL);
> > +   }
> 
> This conditional config file reading is a trap for similar bugs to
> happen again. Is there any reason we should not just mark the command
> RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?

As I plan to slip these patches into Git for Windows v2.11.0, i.e. making
this a last-minute hot fix, I want to err on the side of caution. So I'd
rather keep this conditional (it might regress on the performance front,
or something).

Ciao,
Dscho

Re: [PATCH 2/3] stripspace: respect repository config

[Johannes Schindelin]

Hi Junio,

On Mon, 21 Nov 2016, Junio C Hamano wrote:

> From: Johannes Schindelin 
> 
> The way "git stripspace" reads the configuration was not quite
> correct, in that it forgot to probe for a possibly existing
> repository (note: stripspace is designed to be usable outside the
> repository as well) before doing so.  Due to this, .git/config was
> read only when the command was run from the top-level of the working
> tree.  
> 
> A recent change b9605bc4f2 ("config: only read .git/config from
> configured repos", 2016-09-12) stopped reading the repository-local
> configuration file ".git/config" unless the repository discovery
> process is done, and ".git/config" is no longer read even when run
> from the top-level, which exposed the bug even more.
> 
> When rebasing interactively with a commentChar defined in the
> current repository's config, the help text at the bottom of the edit
> script potentially used an incorrect comment character. This was not
> only funny-looking, but also resulted in tons of warnings like this
> one:
> 
>   Warning: the command isn't recognized in the following line
>- #
> 
> Signed-off-by: Johannes Schindelin 
> Signed-off-by: Junio C Hamano 

Thanks for the corrected commit message!
Dscho

Re: [PATCH 2/3] stripspace: respect repository config

[Duy Nguyen]

On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
 wrote:
> When eff80a9 (Allow custom "comment char", 2013-01-16) taught the
> `stripspace` command to respect the config setting `core.commentChar`,
> it forgot that this variable may be defined in .git/config.
>
> So when rebasing interactively with a commentChar defined in the current
> repository's config, the help text at the bottom of the edit script
> potentially used an incorrect comment character. This was not only
> funny-looking, but also resulted in tons of warnings like this one:
>
> Warning: the command isn't recognized in the following line
>  - #
>
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/stripspace.c  | 4 +++-
>  t/t0030-stripspace.sh | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index 15e716e..1e62a00 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char 
> *prefix)
> if (argc)
> usage_with_options(stripspace_usage, options);
>
> -   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
> +   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> +   setup_git_directory_gently(NULL);
> git_config(git_default_config, NULL);
> +   }

This conditional config file reading is a trap for similar bugs to
happen again. Is there any reason we should not just mark the command
RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?

>
> if (strbuf_read(, 0, 1024) < 0)
> die_errno("could not read the input");
-- 
Duy

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

Junio C Hamano  writes:

> From: Johannes Schindelin 
>
> The way "git stripspace" reads the configuration was not quite
> correct, in that it forgot to probe for a possibly existing
> repository (note: stripspace is designed to be usable outside the
> repository as well) before doing so.  Due to this, .git/config was
> read only when the command was run from the top-level of the working
> tree.  
>
> A recent change b9605bc4f2 ("config: only read .git/config from
> configured repos", 2016-09-12) stopped reading the repository-local
> configuration file ".git/config" unless the repository discovery
> process is done, and ".git/config" is no longer read even when run
> from the top-level, which exposed the bug even more.

The above two paragraphs are rewritten from the original to explain
how this seemed to work (by accident) and its breakage surfaced in
"rebase -i" after b9605bc4f2 ("config: only read .git/config from
configured repos", 2016-09-12) better.  The use of stripspace in
"rebase-i" was done after cd_to_toplevel and it happened to work
before that commit.

Otherwise there is no change from the original.

> When rebasing interactively with a commentChar defined in the
> current repository's config, the help text at the bottom of the edit
> script potentially used an incorrect comment character. This was not
> only funny-looking, but also resulted in tons of warnings like this
> one:
>
>   Warning: the command isn't recognized in the following line
>- #
>
> Signed-off-by: Johannes Schindelin 
> Signed-off-by: Junio C Hamano 
> ---
>  builtin/stripspace.c  | 4 +++-
>  t/t0030-stripspace.sh | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index 15e716ef43..1e62a008cb 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char 
> *prefix)
>   if (argc)
>   usage_with_options(stripspace_usage, options);
>  
> - if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
> + if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> + setup_git_directory_gently(NULL);
>   git_config(git_default_config, NULL);
> + }
>  
>   if (strbuf_read(, 0, 1024) < 0)
>   die_errno("could not read the input");
> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index c1f6411eb2..bbf3e39e3d 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '
>   test_cmp expect actual
>  '
>  
> -test_expect_failure '-c with comment char defined in .git/config' '
> +test_expect_success '-c with comment char defined in .git/config' '
>   test_config core.commentchar = &&
>   printf "= foo\n" >expect &&
>   printf "foo" | (

[PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

From: Johannes Schindelin 

The way "git stripspace" reads the configuration was not quite
correct, in that it forgot to probe for a possibly existing
repository (note: stripspace is designed to be usable outside the
repository as well) before doing so.  Due to this, .git/config was
read only when the command was run from the top-level of the working
tree.  

A recent change b9605bc4f2 ("config: only read .git/config from
configured repos", 2016-09-12) stopped reading the repository-local
configuration file ".git/config" unless the repository discovery
process is done, and ".git/config" is no longer read even when run
from the top-level, which exposed the bug even more.

When rebasing interactively with a commentChar defined in the
current repository's config, the help text at the bottom of the edit
script potentially used an incorrect comment character. This was not
only funny-looking, but also resulted in tons of warnings like this
one:

Warning: the command isn't recognized in the following line
 - #

Signed-off-by: Johannes Schindelin 
Signed-off-by: Junio C Hamano 
---
 builtin/stripspace.c  | 4 +++-
 t/t0030-stripspace.sh | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 15e716ef43..1e62a008cb 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char 
*prefix)
if (argc)
usage_with_options(stripspace_usage, options);
 
-   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
+   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
+   setup_git_directory_gently(NULL);
git_config(git_default_config, NULL);
+   }
 
if (strbuf_read(, 0, 1024) < 0)
die_errno("could not read the input");
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index c1f6411eb2..bbf3e39e3d 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '
test_cmp expect actual
 '
 
-test_expect_failure '-c with comment char defined in .git/config' '
+test_expect_success '-c with comment char defined in .git/config' '
test_config core.commentchar = &&
printf "= foo\n" >expect &&
printf "foo" | (
-- 
2.11.0-rc2-154-g95ba452916

[PATCH 2/3] stripspace: respect repository config

[Johannes Schindelin]

When eff80a9 (Allow custom "comment char", 2013-01-16) taught the
`stripspace` command to respect the config setting `core.commentChar`,
it forgot that this variable may be defined in .git/config.

So when rebasing interactively with a commentChar defined in the current
repository's config, the help text at the bottom of the edit script
potentially used an incorrect comment character. This was not only
funny-looking, but also resulted in tons of warnings like this one:

Warning: the command isn't recognized in the following line
 - #

Signed-off-by: Johannes Schindelin 
---
 builtin/stripspace.c  | 4 +++-
 t/t0030-stripspace.sh | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 15e716e..1e62a00 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char 
*prefix)
if (argc)
usage_with_options(stripspace_usage, options);
 
-   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
+   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
+   setup_git_directory_gently(NULL);
git_config(git_default_config, NULL);
+   }
 
if (strbuf_read(, 0, 1024) < 0)
die_errno("could not read the input");
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 202ac07..67f77df 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '
test_cmp expect actual
 '
 
-test_expect_failure '-c with comment char defined in .git/config' '
+test_expect_success '-c with comment char defined in .git/config' '
test_config core.commentchar = &&
printf "= foo\n" >expect &&
printf "foo" | git stripspace -c >actual &&
-- 
2.10.1.583.g721a9e0