Re: [PATCHv2] rev-parse: add --show-superproject-working-tree
Looks more or less right but invoke "ls-files -z" and reading the \0
delimited output would be easier; otherwise you would have to worry
about c-unquoting the pathname when the submodule is bound at a path
with funny character (like a double-quote) in it.
Also, returning the exact string of the path from the API function is
absolutely the right thing. I however have to wonder if rev-parse need
to do the c-quoting unless it is told to show pathnames in its output
without quoting (perhaps with "-z")? Or are paths from "rev-parse"
(like "--git-dir", "--show-toplevel", etc.) already excempt from the
usual quoting rules---if so, doing puts() and nothing else is fine to
be consistent with the existing practice (in the longer term, I am
sure we would need to revisit so that scripts can handle paths with
funny characters sensibly, but that would be a different topic if
existing ones like "--git-dir" are already unsafe).
Sorry for top-posting (I am not on a terminal right now).
On Tue, Mar 7, 2017 at 4:56 PM, Stefan Beller wrote:
> In some situations it is useful to know if the given repository
> is a submodule of another repository.
>
> Add the flag --show-superproject-working-tree to git-rev-parse
> to make it easy to find out if there is a superproject.
>
> Signed-off-by: Stefan Beller
> ---
>
> * not RFC anymore, but for real this time; including a test and docs :)
>
> * Following Junios advice: there is only one function
> (superproject_exists was dropped) using ls-files.
> (the test actually tests for a staged submodule)
>
> Thanks,
> Stefan
>
>
> Documentation/git-rev-parse.txt | 5 +++
> builtin/rev-parse.c | 7
> submodule.c | 83
> +
> submodule.h | 8
> t/t1500-rev-parse.sh| 14 +++
> 5 files changed, 117 insertions(+)
>
> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index 91c02b8c85..b841bad7c7 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -261,6 +261,11 @@ print a message to stderr and exit with nonzero status.
> --show-toplevel::
> Show the absolute path of the top-level directory.
>
> +--show-superproject-working-tree
> + Show the absolute path of the top-level directory of
> + the superproject. A superproject is a repository that records
> + this repository as a submodule.
> +
> --shared-index-path::
> Show the path to the shared index file in split index mode, or
> empty if not in split-index mode.
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index e08677e559..2549643267 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -12,6 +12,7 @@
> #include "diff.h"
> #include "revision.h"
> #include "split-index.h"
> +#include "submodule.h"
>
> #define DO_REVS1
> #define DO_NOREV 2
> @@ -779,6 +780,12 @@ int cmd_rev_parse(int argc, const char **argv, const
> char *prefix)
> puts(work_tree);
> continue;
> }
> + if (!strcmp(arg, "--show-superproject-working-tree"))
> {
> + const char *superproject =
> get_superproject_working_tree();
> + if (superproject)
> + puts(superproject);
> + continue;
> + }
> if (!strcmp(arg, "--show-prefix")) {
> if (prefix)
> puts(prefix);
> diff --git a/submodule.c b/submodule.c
> index 3b98766a6b..06473d3646 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1514,3 +1514,86 @@ void absorb_git_dir_into_superproject(const char
> *prefix,
> strbuf_release(&sb);
> }
> }
> +
> +const char *get_superproject_working_tree(void)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf sb = STRBUF_INIT;
> + const char *one_up = real_path_if_valid("../");
> + const char *cwd = xgetcwd();
> + const char *ret = NULL;
> + const char *subpath;
> + int code;
> + ssize_t len;
> +
> + if (!is_inside_work_tree())
> + /*
> +* FIXME:
> +* We might have a superproject, but it is harder
> +* to determine.
> +*/
> + return NULL;
> +
> + if (!one_up)
> + return NULL;
> +
> + subpath = relative_path(cwd, one_up, &sb);
> +
> + prepare_submodule_repo_env(&cp.env_array);
> + argv_array_pop(&cp.env_array);
> +
> + argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
> + "ls-files", "--stage", "--full-name", "--", subpath,
> NULL);
> + strbuf_reset(&sb);
> +
> + cp.no_s
Re: [PATCH] t*: avoid using pipes
On Tue, Mar 07, 2017 at 12:52:49PM -0800, Stefan Beller wrote: > On Tue, Mar 7, 2017 at 12:39 PM, Johannes Sixt wrote: > > > Welcome to the Git community! > > > > > Actually, being a *micro* project, it should stay so. Not doing all of the > > changes would leave some tasks for other apprentices to get warm with our > > review process. > > right, so just pick one file. I also wonder if we really want all invocations of git to be marked up in this way. If the primary goal of the test is checking that a certain git command runs successfully and generates the expected output, then I think it is a good candidate for conversion. So in a hunk like this: test_expect_success 'git commit-tree records the correct tree in a commit' ' commit0=$(echo NO | git commit-tree $P) && - tree=$(git show --pretty=raw $commit0 | - sed -n -e "s/^tree //p" -e "/^author /q") && + tree=$(git show --pretty=raw $commit0 >out && + sed -n -e "s/^tree //p" -e "/^author /q"
Re: [PATCHv2] rev-parse: add --show-superproject-working-tree
Stefan Beller writes:
> + if (!strcmp(arg, "--show-superproject-working-tree")) {
> + const char *superproject =
> get_superproject_working_tree();
> + if (superproject)
> + puts(superproject);
> + continue;
> + }
Returning the exact string of the path from the API function is
absolutely the right thing. I however have to wonder if rev-parse
need to do the c-quoting unless it is told to show pathnames in its
output without quoting (perhaps with "-z"). Paths from "rev-parse"
(like "--git-dir", "--show-toplevel", etc.) already are excempt from
the usual quoting rules, so doing puts() and nothing else is fine to
be consistent with the existing practice, but in the longer term, I
am sure we would need to revisit so that scripts can handle paths
with funny characters sensibly, but that would be a different topic
if existing ones like "--git-dir" are already unsafe.
> if (!strcmp(arg, "--show-prefix")) {
> if (prefix)
> puts(prefix);
> diff --git a/submodule.c b/submodule.c
> index 3b98766a6b..06473d3646 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1514,3 +1514,86 @@ void absorb_git_dir_into_superproject(const char
> *prefix,
> strbuf_release(&sb);
> }
> }
> +
> +const char *get_superproject_working_tree(void)
> +{
> +...
> + argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
> + "ls-files", "--stage", "--full-name", "--", subpath,
> NULL);
> + strbuf_reset(&sb);
> +...
> + if (starts_with(sb.buf, "16")) {
> + int super_sub_len;
> + int cwd_len = strlen(cwd);
> + char *super_sub, *super_wt;
> +
> + /*
> + * There is a superproject having this repo as a submodule.
> + * The format is SP SP TAB LF,
> + * First remove LF, then skip up to \t.
> + */
Looks more or less right but invoke "ls-files -z" and reading the \0
delimited output would be easier; otherwise you would have to worry
about c-unquoting the pathname when the submodule is bound at a path
with funny character (like a double-quote) in it.
Re: [Request for Documentation] Differentiate signed (commits/tags/pushes)
On Tue, Mar 07, 2017 at 02:19:19PM -0800, Stefan Beller wrote: > On Tue, Mar 7, 2017 at 1:23 AM, Jeff King wrote: > > On Mon, Mar 06, 2017 at 11:59:24AM -0800, Stefan Beller wrote: > > > >> What is the difference between signed commits and tags? > >> (Not from a technical perspective, but for the end user) > > > > I think git has never really tried to assign any _meaning_ to the > > signatures. It implements the technical bits and leaves it up to the > > user and their workflows to decide what a given signature means. > > That is a nihilistic approach? ;) To some degree. :) I think it is less that we believe in nothing, but that Git has traditionally been a toolkit on which you could build a variety of workflows. We wouldn't want to constrain the low-level tools by building too much policy logic into them. And IMHO, nobody has really written the high-level tools for signing. I would be happy if somebody wanted to do so. Either separate tools, or a suite of options and config that could be used to make the existing tools help enforce certain policies. E.g., if "git log --show-signature" were to complain that the signer id does not match the committer ident, when a certain config option is set. Having a git-trust-model(7) manpage is probably a good idea, but I think it's nicer still if users can adopt the trust model with the flip of a switch. > Off list I was told > "just look at Documentation/technical/signature-format.txt; > it explains all different things that you can sign or have signed > stuff". But as an end user I refuse to look at that. ;) I agree that end users should not have to look at it. But I also think it doesn't actually discuss the policy options (i.e., what the signatures could "mean"). AFAIK there is not a good discussion of that anywhere. > > Signing individual commits _could_ convey meaning (2), but "all history > > leading up" part is unlikely to be something that the signer needs to > > enforce on every commit. > > I was told signed commits could act as a poor mans > push certificate (again off list :/). Yeah, they could. But you should probably just use push certificates. Which _also_ are missing the high-level workflow bits; I'd be happy to get GitHub to start recording push signatures somewhere if people know what they would want to do with them. The idea was discussed of sticking them in git-notes. But I wonder if it would be useful to hand them out during upload-pack, so that somebody fetching has a cryptographic chain back to the original pusher (rather than trusting the intermediate server). I think that is slightly complicated because each push certificate only mentions the refs that were pushed. So verifying the ref state for N refs might involve up to N separate push certificates (i.e., the last one that touched each ref, not just the last one overall). > > In my opinion, the most useful meaning for commit-signing is simply to > > cryptographically certify the identity of the committer. We don't > > compare the GPG key ident to the "committer" field, but IMHO that would > > be a reasonable optional feature for verify-commit (I say optional > > because we're starting to assign semantics now). > > So the signed commit focuses on the committer instead of the content > (which is what tags are rather used for) ? I think it's more subtle than that, and gets into the "is a commit a snapshot or a diff" question. We all know that technically the commit object points to a snapshot of the tree. But it also points to a parent, and I think what is interesting here is the change between the parent's tree and the commit's tree. So I would take a commit signature to mean that you are the author of the changes moving from $commit^ to $commit, without making any specific attestation of the content in $commit^. IOW, you are really signing the changes and your commit message. > > I think one of the reasons kernel (and git) developers aren't that > > interested in signed commits is that they're not really that interesting > > in a patch workflow. You're verifying the committer, who in this project > > is invariably Junio, and we just take his word that whatever is in the > > "author" field is reasonable. > > Well in such a workflow Junio could also sign the tip-commits of > pu/next before pushing, such that we can trust it was really him doing > the maintenance work and not his evil twin. Yes, he could. He could also force-push a tag for the current value (although git does not handle tag-overwriting very well). That's sort of what push-certificates were meant to do a better job of. > > So I don't think it's just a checkbox feature. It's a useful thing for > > certain workflows that really want to be able to attribute individual > > commits with cryptographic strength. > > "certain workflows". :( > > See, I really like reading e.g. the "On Re-tagging" section of git-tag > as it doesn't hand wave around the decisions to make. OK, less hand-waving. Imagine you are a compan
Representative Needed.
Hello, We want you to become our agent/representative. If you are interested in becoming our representative contact us. Regards, Jiang Liang Chairman Geo-Jade Petroleum
[PATCH] t*: avoid using pipes
Hi, I'm Prathamesh Chavan. As a part of my micropraoject I have been working on "Avoid pipes for git related commands in test suites". I tried sending the patch, but it got blocked since the mail contained more than 100 000 characters. Hence, I have made the required changes in branch "micro-proj" and you may find it in 'git' repository on my github account. My user name is: pratham-pc. Please review the changes. Thanks.
Re: Crash on MSYS2 with GIT_WORK_TREE
Johannes Schindelin writes: > The problem is actually even worse: On *Linux*, this happens: > > $ GIT_WORK_TREE=c:/invalid git rev-parse HEAD > Segmentation fault (core dumped) > > The reason is this: when set_git_work_tree() was converted from using > xstrdup(real_path()) to real_pathdup(), we completely missed the fact that > the former passed die_on_error = 1 to strbuf_realpath(), while the latter > passed die_on_error = 0. As a consequence, work_tree can be NULL now, and > the current code does not expect set_git_work_tree() to return > successfully after setting work_tree to NULL. Ouch. > Brandon, I have a hunch that pretty much all of the xstrdup(real_path()) > -> real_pathdup() sites have a problem now. The previous contract was that > real_path() would die() if the passed path is invalid. The new contract is > that real_pathdup() returns NULL in such a case. OK, so it appears that we'd better audit all the callsites of real_pathdup() and see if anybody _assumes_ that the return values are not NULL. They all need fixing. Thanks for digging it through to the root cause.
[PATCH 0/6] deadlock regression in v2.11.0 with failed mkdtemp
On Tue, Mar 07, 2017 at 12:14:06PM +0100, Horst Schirmeier wrote: > On Tue, 07 Mar 2017, Horst Schirmeier wrote: > > I observe a regression that seems to have been introduced between > > v2.10.0 and v2.11.0. When I try to push into a repository on the local > > filesystem that belongs to another user and has not explicitly been > > prepared for shared use, v2.11.0 shows some of the usual diagnostic > > output and then freezes instead of announcing why it failed to push. > > Bisecting points to v2.10.1-373-g722ff7f: > > 722ff7f876c8a2ad99c42434f58af098e61b96e8 is the first bad commit > commit 722ff7f876c8a2ad99c42434f58af098e61b96e8 > Author: Jeff King > Date: Mon Oct 3 16:49:14 2016 -0400 > > receive-pack: quarantine objects until pre-receive accepts Thanks, I was able to reproduce easily with: git init --bare foo.git chown -R nobody foo.git git push foo.git HEAD Here's a series to fix it. The first patch addresses the deadlock. The rest try to improve the output on the client side. With v2.10.0, this case looks like: $ git push ~/tmp/foo.git HEAD Counting objects: 209837, done. Delta compression using up to 8 threads. Compressing objects: 100% (52180/52180), done. remote: fatal: Unable to create temporary file '/home/peff/tmp/foo.git/./objects/pack/tmp_pack_XX': Permission denied error: failed to push some refs to '/home/peff/tmp/foo.git' With just the first patch applied, you get just: $ git push ~/tmp/foo.git HEAD Counting objects: 210973, done. Delta compression using up to 8 threads. Compressing objects: 100% (52799/52799), done. error: failed to push some refs to '/home/peff/tmp/foo.git' Yuck. In the original, the error was generated by the child index-pack, and we relayed it over the sideband. But we don't even get as far as running index-pack in the newer version; we fail trying to make the tmpdir. The error ends up in the "unpack" protocol field, but the client side does a bad job of showing that. With the rest of the patches, it looks like: $ git push ~/tmp/foo.git HEAD Counting objects: 210973, done. Delta compression using up to 8 threads. Compressing objects: 100% (52799/52799), done. error: remote unpack failed: unable to create temporary object directory error: failed to push some refs to '/home/peff/tmp/foo.git' Compared to v2.10.0, that omits the name of the directory and the errno value (because receive-pack does not send them). That potentially makes debugging harder, but arguably it's the right thing to do. On a remote site, those details aren't really the client's business. But if people feel strongly, we could add them back into this error code path. So the first patch is a no-brainer and should go to maint. The rest can be applied separately if need be. [1/6]: receive-pack: fix deadlock when we cannot create tmpdir [2/6]: send-pack: extract parsing of "unpack" response [3/6]: send-pack: use skip_prefix for parsing unpack status [4/6]: send-pack: improve unpack-status error messages [5/6]: send-pack: read "unpack" status even on pack-objects failure [6/6]: send-pack: report signal death of pack-objects builtin/receive-pack.c | 5 - send-pack.c| 46 -- 2 files changed, 40 insertions(+), 11 deletions(-) -Peff
Re: [PATCH v4 04/10] setup_git_directory_1(): avoid changing global state
On 03/08, Johannes Schindelin wrote:
> Hi Brandon,
>
> On Tue, 7 Mar 2017, Brandon Williams wrote:
>
> > On 03/07, Johannes Schindelin wrote:
> > > const char *setup_git_directory_gently(int *nongit_ok)
> > > {
> > > + struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, gitdir =
> > > STRBUF_INIT;
> >
> > I couldn't see any strbuf_release() calls for these strbufs so there may
> > be some memory leaking here.
>
> You are correct, of course. Something like this may work:
Yep that should fix it!
>
> -- snipsnap --
> diff --git a/setup.c b/setup.c
> index 9118b48590a..c822582b96e 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1027,6 +1027,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
> case GIT_DIR_HIT_MOUNT_POINT:
> if (nongit_ok) {
> *nongit_ok = 1;
> + strbuf_release(&cwd);
> + strbuf_release(&dir);
> return NULL;
> }
> die(_("Not a git repository (or any parent up to mount point
> %s)\n"
> @@ -1044,6 +1046,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
> startup_info->have_repository = !nongit_ok || !*nongit_ok;
> startup_info->prefix = prefix;
>
> + strbuf_release(&cwd);
> + strbuf_release(&dir);
> + strbuf_release(&gitdir);
> +
> return prefix;
> }
>
--
Brandon Williams
Re: Crash on MSYS2 with GIT_WORK_TREE
On 03/08, Johannes Schindelin wrote:
> Hi valtron,
>
> On Wed, 8 Mar 2017, Johannes Schindelin wrote:
>
> > On Tue, 7 Mar 2017, valtron wrote:
> >
> > > When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git
> > > commands crash:
> > >
> > > C:\repo>set GIT_WORK_TREE=C:/repo
> > > C:\repo>git rev-parse HEAD
> > > 1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping
> > > stack trace to git.exe.stackdump
> >
> > [...]
> >
> > In any case, this problem is squarely within the MSYS2 runtime. It has
> > nothing to do with Git except for the motivation to set an environment
> > variable to an absolute path as you outlined.
>
> Oh boy was I *wrong*! I take that back and apologize for my premature
> verdict.
>
> It is true that you should not set GIT_WORKTREE=c:/repo if you want to
> work with MSYS2 Git because MSYS2 expects pseudo Unix paths, i.e. /c/repo,
> and it will simply try to guess correctly and convert Windows paths with
> drive letters and backslashes to that form.
>
> But that does not excuse a crash.
>
> The problem is actually even worse: On *Linux*, this happens:
>
> $ GIT_WORK_TREE=c:/invalid git rev-parse HEAD
> Segmentation fault (core dumped)
>
> The reason is this: when set_git_work_tree() was converted from using
> xstrdup(real_path()) to real_pathdup(), we completely missed the fact that
> the former passed die_on_error = 1 to strbuf_realpath(), while the latter
> passed die_on_error = 0. As a consequence, work_tree can be NULL now, and
> the current code does not expect set_git_work_tree() to return
> successfully after setting work_tree to NULL.
>
> I Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers use
> real_pathdup and strbuf_realpath, 2016-12-12).
>
> Brandon, I have a hunch that pretty much all of the xstrdup(real_path())
> -> real_pathdup() sites have a problem now. The previous contract was that
> real_path() would die() if the passed path is invalid. The new contract is
> that real_pathdup() returns NULL in such a case. I believe that the
> following call sites are problematic in particular:
Welp, looks like I missed that when I made the conversion. You're
right, the semantics of getting the real_path were changed which would
cause a NULL to be returned instead of the program exiting with a call
to die().
After a cursory look at your patch, I think all of your changes look
sane. I would have to take a closer look at the call sites to see if
each caller would need to die or not. I'm assuming you took a quick
glace to make your decision about each call site?
>
> builtin/init-db.c: init_db():
> char *original_git_dir = real_pathdup(git_dir);
>
> builtin/init-db.c: cmd_init_db():
> real_git_dir = real_pathdup(real_git_dir);
> ...
> git_work_tree_cfg = real_pathdup(rel);
>
> environment.c: set_git_work_tree():
> work_tree = real_pathdup(new_work_tree);
>
> setup.c: setup_discovered_git_dir():
> gitdir = real_pathdup(gitdir);
>
> submodule.c: connect_work_tree_and_git_dir():
> const char *real_work_tree = real_pathdup(work_tree);
>
> transport.c: refs_from_alternate_cb():
> other = real_pathdup(e->path);
>
> worktree.c: find_worktree():
> path = real_pathdup(arg);
>
> I verified that all calls are still there, except for the submodule.c one
> which simply moved to dir.c and the transport.c one which apparently now
> no longer die()s but simply ignores non-existing paths now.
>
> That leaves six places to patch, methinks... This diff may serve as an
> initial version, but I have not really had a deep look at all call sites
> (and it is an unwise idea to trust me at this hour anyway, look at the
> time when I sent this mail):
>
> -- snipsnap --
> diff --git a/abspath.c b/abspath.c
> index 2f0c26e0e2c..b02e068aa34 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)
> return strbuf_realpath(&realpath, path, 0);
> }
>
> -char *real_pathdup(const char *path)
> +char *real_pathdup(const char *path, int die_on_error)
> {
> struct strbuf realpath = STRBUF_INIT;
> char *retval = NULL;
>
> - if (strbuf_realpath(&realpath, path, 0))
> + if (strbuf_realpath(&realpath, path, die_on_error))
> retval = strbuf_detach(&realpath, NULL);
>
> strbuf_release(&realpath);
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 1d4d6a00789..8a6acb0ec69 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
> {
> int reinit;
> int exist_ok = flags & INIT_DB_EXIST_OK;
> - char *original_git_dir = real_pathdup(git_dir);
> + char *original_git_dir = real_pathdup(git_dir, 1);
>
> if (real_git_dir) {
> struct stat st;
> @@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char
> *prefix)
> argc = parse_
Re: Crash on MSYS2 with GIT_WORK_TREE
Hi valtron,
On Wed, 8 Mar 2017, Johannes Schindelin wrote:
> On Tue, 7 Mar 2017, valtron wrote:
>
> > When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git
> > commands crash:
> >
> > C:\repo>set GIT_WORK_TREE=C:/repo
> > C:\repo>git rev-parse HEAD
> > 1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping
> > stack trace to git.exe.stackdump
>
> [...]
>
> In any case, this problem is squarely within the MSYS2 runtime. It has
> nothing to do with Git except for the motivation to set an environment
> variable to an absolute path as you outlined.
Oh boy was I *wrong*! I take that back and apologize for my premature
verdict.
It is true that you should not set GIT_WORKTREE=c:/repo if you want to
work with MSYS2 Git because MSYS2 expects pseudo Unix paths, i.e. /c/repo,
and it will simply try to guess correctly and convert Windows paths with
drive letters and backslashes to that form.
But that does not excuse a crash.
The problem is actually even worse: On *Linux*, this happens:
$ GIT_WORK_TREE=c:/invalid git rev-parse HEAD
Segmentation fault (core dumped)
The reason is this: when set_git_work_tree() was converted from using
xstrdup(real_path()) to real_pathdup(), we completely missed the fact that
the former passed die_on_error = 1 to strbuf_realpath(), while the latter
passed die_on_error = 0. As a consequence, work_tree can be NULL now, and
the current code does not expect set_git_work_tree() to return
successfully after setting work_tree to NULL.
I Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers use
real_pathdup and strbuf_realpath, 2016-12-12).
Brandon, I have a hunch that pretty much all of the xstrdup(real_path())
-> real_pathdup() sites have a problem now. The previous contract was that
real_path() would die() if the passed path is invalid. The new contract is
that real_pathdup() returns NULL in such a case. I believe that the
following call sites are problematic in particular:
builtin/init-db.c: init_db():
char *original_git_dir = real_pathdup(git_dir);
builtin/init-db.c: cmd_init_db():
real_git_dir = real_pathdup(real_git_dir);
...
git_work_tree_cfg = real_pathdup(rel);
environment.c: set_git_work_tree():
work_tree = real_pathdup(new_work_tree);
setup.c: setup_discovered_git_dir():
gitdir = real_pathdup(gitdir);
submodule.c: connect_work_tree_and_git_dir():
const char *real_work_tree = real_pathdup(work_tree);
transport.c: refs_from_alternate_cb():
other = real_pathdup(e->path);
worktree.c: find_worktree():
path = real_pathdup(arg);
I verified that all calls are still there, except for the submodule.c one
which simply moved to dir.c and the transport.c one which apparently now
no longer die()s but simply ignores non-existing paths now.
That leaves six places to patch, methinks... This diff may serve as an
initial version, but I have not really had a deep look at all call sites
(and it is an unwise idea to trust me at this hour anyway, look at the
time when I sent this mail):
-- snipsnap --
diff --git a/abspath.c b/abspath.c
index 2f0c26e0e2c..b02e068aa34 100644
--- a/abspath.c
+++ b/abspath.c
@@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)
return strbuf_realpath(&realpath, path, 0);
}
-char *real_pathdup(const char *path)
+char *real_pathdup(const char *path, int die_on_error)
{
struct strbuf realpath = STRBUF_INIT;
char *retval = NULL;
- if (strbuf_realpath(&realpath, path, 0))
+ if (strbuf_realpath(&realpath, path, die_on_error))
retval = strbuf_detach(&realpath, NULL);
strbuf_release(&realpath);
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 1d4d6a00789..8a6acb0ec69 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
{
int reinit;
int exist_ok = flags & INIT_DB_EXIST_OK;
- char *original_git_dir = real_pathdup(git_dir);
+ char *original_git_dir = real_pathdup(git_dir, 1);
if (real_git_dir) {
struct stat st;
@@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char
*prefix)
argc = parse_options(argc, argv, prefix, init_db_options,
init_db_usage, 0);
if (real_git_dir && !is_absolute_path(real_git_dir))
- real_git_dir = real_pathdup(real_git_dir);
+ real_git_dir = real_pathdup(real_git_dir, 1);
if (argc == 1) {
int mkdir_tried = 0;
@@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char
*prefix)
const char *git_dir_parent = strrchr(git_dir, '/');
if (git_dir_parent) {
char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
- git_work_tree_cfg = real_pathdup(rel);
+ git_work_tree_cfg = real_pathdup(r
Re: [PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.
Stefan Beller writes:
> submodule_from_ce returns always NULL, when such flag is not given.
> From 10/18:
>
> +const struct submodule *submodule_from_ce(const struct cache_entry *ce)
> +{
> + if (!S_ISGITLINK(ce->ce_mode))
> + return NULL;
> +
> + if (!should_update_submodules())
> + return NULL;
> +
> + return submodule_from_path(null_sha1, ce->name);
> +}
>
> should_update_submodules is always false if such a flag is not set,
> such that we end up in the else case which is literally the same as
> the removed lines (they are just indented).
I see.
I didn't think a function this deep in the callchain that does not
take any parameter could possibly change the behaviour based on the
end-user input. I was expecting that such a state (i.e. are we
recursive? are we allowed to forcibly update the working tree
files?) would be kept part of something like "struct checkout" and
passed around the callchain.
That was why I didn't look at how that function answers "should
update?" question, and got puzzled. Because it would imply there is
some hidden state that is accessible by everybody--a global variable
or something--which would point at a deeper design issue.
Re: Crash on MSYS2 with GIT_WORK_TREE
Hi valtron,
just to set the record straight on a few of my suggestions that turned out
to be incorrect:
On Wed, 8 Mar 2017, Johannes Schindelin wrote:
> On Tue, 7 Mar 2017, valtron wrote:
>
> > Stacktrace from GDB (on git-rev-parse) is:
> >
> > #0 0x00018019634d in strcmp (s1=0x600062080 "/c/repo", s2=0x0)
> >at
> > /msys_scripts/msys2-runtime/src/msys2-runtime/newlib/libc/string/strcmp.c:83
> > #1 0x0001005239f1 in ?? ()
> > #2 0x000100523f36 in ?? ()
> > #3 0x00010046c6fa in ?? ()
> > #4 0x000100401b6d in ?? ()
> > #5 0x000100401e4b in ?? ()
> > #6 0x000100563593 in ?? ()
> > #7 0x000180047c37 in _cygwin_exit_return ()
> >at
> > /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/dcrt0.cc:1031
> > #8 0x000180045873 in _cygtls::call2 (this=0xce00,
> > func=0x180046bd0 , arg=0x0,
> >buf=buf@entry=0xcdf0) at
> > /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:40
> > #9 0x000180045924 in _cygtls::call (func=,
> > arg=)
> >at
> > /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:27
> > #10 0x in ?? ()
> > Backtrace stopped: previous frame inner to this frame (corrupt stack?)
>
> This may be the wrong thread, though. You can find out what other threads
> there are with `info threads` and switch by `thread `, the
> backtrace(s) of the other thread(s) may be informative.
It was actually the correct thread.
> Please also note that installing the `msys2-runtime-devel` package via
> Pacman may be able to get you nicer symbols.
I was wrong. The problem here is git.exe: you would have to uncomment the
'#options("debug","!strip")' line in the git/PKGBUILD file in
https://github.com/Alexpux/MSYS2-packages before rebuilding the package
[*1*], and then reinstalling it (which increases the installed size by
94.82MB to 119.63MB due to the unstripped symbols). But then gdb would
find the symbols alright and you'd see that the crash happens in
#1 0x00010051aecb in setup_explicit_git_dir
(gitdirenv=gitdirenv@entry=0x1005a00cc ".git",
cwd=cwd@entry=0x10055e970 , nongit_ok=nongit_ok@entry=0x0) at
setup.c:669
669 if (!strcmp(cwd->buf, worktree)) { /* cwd == worktree */
and worktree indeed was NULL.
Ciao,
Johannes
Re: [PATCH 16/18] entry.c: update submodules when interesting
Stefan Beller writes: >> In addition to mkdir(), would we also need the .git file that point >> into superproject's .git/modules/ too? > > The move_head function takes care of it > Both creation as well as deletion are handled in the move_head function, > when either new or old is NULL. Oh, if it does the creation of directory and adjusting of .git gitfile, and handles other anomalies, I have no problem with it. It was just that this codepath tried to handle some kind of anomaly (namely, the user originally thought the submodule needs to be changed to a regular file and then changed mind and wants to restore it from the index) and yet not doing the full anomaly handling (like "not even an empty directory exists" case) that confused me.
Re: Crash on MSYS2 with GIT_WORK_TREE
Hi Johannes,
Thanks for looking at this! Yes, it's 2.12.0, sorry for the typo.
I only ran into this because of git-gui, where I eventually tracked it
down to line 1330:
set env(GIT_WORK_TREE) $_gitworktree
With that line commented out, it works. I'll look into why git-gui
sets it to a windows-path-with-forward-slashes, but that's a separate
issue from the crash. Also, from the stack trace, I think git is still
able to understand the path, since it appears to correctly convert it
to /c/repo, but I might be wrong since I haven't look at the code.
On Tue, Mar 7, 2017 at 5:51 PM, Johannes Schindelin
wrote:
> Hi valtron,
>
> On Wed, 8 Mar 2017, Johannes Schindelin wrote:
>
>> On Tue, 7 Mar 2017, valtron wrote:
>>
>> > When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git
>> > commands crash:
>> >
>> > C:\repo>set GIT_WORK_TREE=C:/repo
>> > C:\repo>git rev-parse HEAD
>> > 1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping
>> > stack trace to git.exe.stackdump
>>
>> [...]
>>
>> In any case, this problem is squarely within the MSYS2 runtime. It has
>> nothing to do with Git except for the motivation to set an environment
>> variable to an absolute path as you outlined.
>
> Oh boy was I *wrong*! I take that back and apologize for my premature
> verdict.
>
> It is true that you should not set GIT_WORKTREE=c:/repo if you want to
> work with MSYS2 Git because MSYS2 expects pseudo Unix paths, i.e. /c/repo,
> and it will simply try to guess correctly and convert Windows paths with
> drive letters and backslashes to that form.
>
> But that does not excuse a crash.
>
> The problem is actually even worse: On *Linux*, this happens:
>
> $ GIT_WORK_TREE=c:/invalid git rev-parse HEAD
> Segmentation fault (core dumped)
>
> The reason is this: when set_git_work_tree() was converted from using
> xstrdup(real_path()) to real_pathdup(), we completely missed the fact that
> the former passed die_on_error = 1 to strbuf_realpath(), while the latter
> passed die_on_error = 0. As a consequence, work_tree can be NULL now, and
> the current code does not expect set_git_work_tree() to return
> successfully after setting work_tree to NULL.
>
> I Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers use
> real_pathdup and strbuf_realpath, 2016-12-12).
>
> Brandon, I have a hunch that pretty much all of the xstrdup(real_path())
> -> real_pathdup() sites have a problem now. The previous contract was that
> real_path() would die() if the passed path is invalid. The new contract is
> that real_pathdup() returns NULL in such a case. I believe that the
> following call sites are problematic in particular:
>
> builtin/init-db.c: init_db():
> char *original_git_dir = real_pathdup(git_dir);
>
> builtin/init-db.c: cmd_init_db():
> real_git_dir = real_pathdup(real_git_dir);
> ...
> git_work_tree_cfg = real_pathdup(rel);
>
> environment.c: set_git_work_tree():
> work_tree = real_pathdup(new_work_tree);
>
> setup.c: setup_discovered_git_dir():
> gitdir = real_pathdup(gitdir);
>
> submodule.c: connect_work_tree_and_git_dir():
> const char *real_work_tree = real_pathdup(work_tree);
>
> transport.c: refs_from_alternate_cb():
> other = real_pathdup(e->path);
>
> worktree.c: find_worktree():
> path = real_pathdup(arg);
>
> I verified that all calls are still there, except for the submodule.c one
> which simply moved to dir.c and the transport.c one which apparently now
> no longer die()s but simply ignores non-existing paths now.
>
> That leaves six places to patch, methinks... This diff may serve as an
> initial version, but I have not really had a deep look at all call sites
> (and it is an unwise idea to trust me at this hour anyway, look at the
> time when I sent this mail):
>
> -- snipsnap --
> diff --git a/abspath.c b/abspath.c
> index 2f0c26e0e2c..b02e068aa34 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)
> return strbuf_realpath(&realpath, path, 0);
> }
>
> -char *real_pathdup(const char *path)
> +char *real_pathdup(const char *path, int die_on_error)
> {
> struct strbuf realpath = STRBUF_INIT;
> char *retval = NULL;
>
> - if (strbuf_realpath(&realpath, path, 0))
> + if (strbuf_realpath(&realpath, path, die_on_error))
> retval = strbuf_detach(&realpath, NULL);
>
> strbuf_release(&realpath);
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 1d4d6a00789..8a6acb0ec69 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
> {
> int reinit;
> int exist_ok = flags & INIT_DB_EXIST_OK;
> - char *original_git_dir = real_pathdup(git_dir);
> + char *original_git_dir = real_pathdup(git_dir, 1);
>
> if (real_git_dir) {
> struct stat st;
> @@ -48
[PATCHv2] rev-parse: add --show-superproject-working-tree
In some situations it is useful to know if the given repository
is a submodule of another repository.
Add the flag --show-superproject-working-tree to git-rev-parse
to make it easy to find out if there is a superproject.
Signed-off-by: Stefan Beller
---
* not RFC anymore, but for real this time; including a test and docs :)
* Following Junios advice: there is only one function
(superproject_exists was dropped) using ls-files.
(the test actually tests for a staged submodule)
Thanks,
Stefan
Documentation/git-rev-parse.txt | 5 +++
builtin/rev-parse.c | 7
submodule.c | 83 +
submodule.h | 8
t/t1500-rev-parse.sh| 14 +++
5 files changed, 117 insertions(+)
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 91c02b8c85..b841bad7c7 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -261,6 +261,11 @@ print a message to stderr and exit with nonzero status.
--show-toplevel::
Show the absolute path of the top-level directory.
+--show-superproject-working-tree
+ Show the absolute path of the top-level directory of
+ the superproject. A superproject is a repository that records
+ this repository as a submodule.
+
--shared-index-path::
Show the path to the shared index file in split index mode, or
empty if not in split-index mode.
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index e08677e559..2549643267 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -12,6 +12,7 @@
#include "diff.h"
#include "revision.h"
#include "split-index.h"
+#include "submodule.h"
#define DO_REVS1
#define DO_NOREV 2
@@ -779,6 +780,12 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
puts(work_tree);
continue;
}
+ if (!strcmp(arg, "--show-superproject-working-tree")) {
+ const char *superproject =
get_superproject_working_tree();
+ if (superproject)
+ puts(superproject);
+ continue;
+ }
if (!strcmp(arg, "--show-prefix")) {
if (prefix)
puts(prefix);
diff --git a/submodule.c b/submodule.c
index 3b98766a6b..06473d3646 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1514,3 +1514,86 @@ void absorb_git_dir_into_superproject(const char *prefix,
strbuf_release(&sb);
}
}
+
+const char *get_superproject_working_tree(void)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf sb = STRBUF_INIT;
+ const char *one_up = real_path_if_valid("../");
+ const char *cwd = xgetcwd();
+ const char *ret = NULL;
+ const char *subpath;
+ int code;
+ ssize_t len;
+
+ if (!is_inside_work_tree())
+ /*
+* FIXME:
+* We might have a superproject, but it is harder
+* to determine.
+*/
+ return NULL;
+
+ if (!one_up)
+ return NULL;
+
+ subpath = relative_path(cwd, one_up, &sb);
+
+ prepare_submodule_repo_env(&cp.env_array);
+ argv_array_pop(&cp.env_array);
+
+ argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
+ "ls-files", "--stage", "--full-name", "--", subpath,
NULL);
+ strbuf_reset(&sb);
+
+ cp.no_stdin = 1;
+ cp.no_stderr = 1;
+ cp.out = -1;
+ cp.git_cmd = 1;
+
+ if (start_command(&cp))
+ die(_("could not start ls-files in .."));
+
+ len = strbuf_read(&sb, cp.out, PATH_MAX);
+ close(cp.out);
+
+ if (starts_with(sb.buf, "16")) {
+ int super_sub_len;
+ int cwd_len = strlen(cwd);
+ char *super_sub, *super_wt;
+
+ /*
+* There is a superproject having this repo as a submodule.
+* The format is SP SP TAB LF,
+* First remove LF, then skip up to \t.
+*/
+ strbuf_rtrim(&sb);
+ super_sub = strchr(sb.buf, '\t') + 1;
+
+ super_sub_len = sb.buf + sb.len - super_sub;
+ if (super_sub_len > cwd_len ||
+ strcmp(&cwd[cwd_len - super_sub_len], super_sub))
+ die (_("BUG: returned path string doesn't match cwd?"));
+
+ super_wt = xstrdup(cwd);
+ super_wt[cwd_len - super_sub_len] = '\0';
+
+ ret = real_path(super_wt);
+
+ free(super_wt);
+ }
+ strbuf_release(&sb);
+
+ code = finish_command(&cp);
+
+
Re: [PATCH v4 04/10] setup_git_directory_1(): avoid changing global state
Hi Brandon,
On Tue, 7 Mar 2017, Brandon Williams wrote:
> On 03/07, Johannes Schindelin wrote:
> > const char *setup_git_directory_gently(int *nongit_ok)
> > {
> > + struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, gitdir =
> > STRBUF_INIT;
>
> I couldn't see any strbuf_release() calls for these strbufs so there may
> be some memory leaking here.
You are correct, of course. Something like this may work:
-- snipsnap --
diff --git a/setup.c b/setup.c
index 9118b48590a..c822582b96e 100644
--- a/setup.c
+++ b/setup.c
@@ -1027,6 +1027,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
case GIT_DIR_HIT_MOUNT_POINT:
if (nongit_ok) {
*nongit_ok = 1;
+ strbuf_release(&cwd);
+ strbuf_release(&dir);
return NULL;
}
die(_("Not a git repository (or any parent up to mount point
%s)\n"
@@ -1044,6 +1046,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
startup_info->have_repository = !nongit_ok || !*nongit_ok;
startup_info->prefix = prefix;
+ strbuf_release(&cwd);
+ strbuf_release(&dir);
+ strbuf_release(&gitdir);
+
return prefix;
}
Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree
Stefan Beller writes:
> On Tue, Mar 7, 2017 at 10:44 AM, Junio C Hamano wrote:
>
>> So perhaps your superproject_exists() helper can be eliminated
>
> That is what I had originally, but I assumed a strict helper function
> for "existence of the superproject" would be interesting in the future,
> e.g. for get_superproject_git_dir, or on its own. There was an attempt
> to have the shell prompt indicate if you are in a submodule,
> which would not need to know the worktree or git dir of the
> superproject, but only its existence.
>
>> instead coded in get_superproject_working_tree() in place to do:
>>
>> - xgetcwd() to get "/local/repo/super/sub/dir".
>
> Did you mean .../super/dir/sub ?
I meant "/local/repo/super/sub/dir". I am using this case to
illustrate: the root of the superproject is at /local/repo/super,
its submodule we are interested in is at sub/dir, and the function
is working inside the submodule--after the repository discovery
moves the cwd, xgetcwd() would give the root of the working tree of
the submodule, which is at "/local/repo/super/sub/dir".
And that would give us "dir" by taking that as relative to its "../"
>> - relative_path() to get "dir".
>
> ok.
indeed.
>> - ask "ls-{tree,files} --full-name HEAD dir" to get "16"
>> and "sub/dir".
>
> "ls-files --stage --full-name" to get
> 16 ... dir/sub
Yeah, also when usihng ls-files you obviously do not give HEAD but
you do give "dir" as the pathspec. And you get "sub/dir" as the
result.
Re: [Request for Documentation] Differentiate signed (commits/tags/pushes)
On Tue, Mar 7, 2017 at 1:23 AM, Jeff King wrote: > On Mon, Mar 06, 2017 at 11:59:24AM -0800, Stefan Beller wrote: > >> What is the difference between signed commits and tags? >> (Not from a technical perspective, but for the end user) > > I think git has never really tried to assign any _meaning_ to the > signatures. It implements the technical bits and leaves it up to the > user and their workflows to decide what a given signature means. That is a nihilistic approach? ;) As a user I would like to know what I can do with such a signed commit. And I happen to be an experienced user in the sense that I know about git tag --sign as well as git verify-tag; further reading of these two man pages tells me I can even use git-tag to verify a tag. So looking for the verify option in git-commit for signed commits... well, no. Ah! git-verify-commit it is. I assumed to have most discussion in git-tag or at least a pointer from there to further reading. In an ideal world we might have a manpage git-trust-model(7), that explains different workflows and when certain signing mechanisms make sense and what they protect us from. I might write such a man page (after I get the gitmodules page done). Off list I was told "just look at Documentation/technical/signature-format.txt; it explains all different things that you can sign or have signed stuff". But as an end user I refuse to look at that. ;) > > People generally seem to take tag signatures to mean one (or both) of: > > 1. Certifying that the tree contents at that tag are the official, > signed release contents (i.e., this is version 1.0, and I'm the > maintainer, so believe me). > > 2. Certifying that all the history leading up to that tag is > "official" in some sense (i.e., I'm the maintainer, and this was > the tip of my git tree at the time I tagged the release). > > Signing individual commits _could_ convey meaning (2), but "all history > leading up" part is unlikely to be something that the signer needs to > enforce on every commit. I was told signed commits could act as a poor mans push certificate (again off list :/). > In my opinion, the most useful meaning for commit-signing is simply to > cryptographically certify the identity of the committer. We don't > compare the GPG key ident to the "committer" field, but IMHO that would > be a reasonable optional feature for verify-commit (I say optional > because we're starting to assign semantics now). So the signed commit focuses on the committer instead of the content (which is what tags are rather used for) ? > I think one of the reasons kernel (and git) developers aren't that > interested in signed commits is that they're not really that interesting > in a patch workflow. You're verifying the committer, who in this project > is invariably Junio, and we just take his word that whatever is in the > "author" field is reasonable. Well in such a workflow Junio could also sign the tip-commits of pu/next before pushing, such that we can trust it was really him doing the maintenance work and not his evil twin. > But for a project whose workflow is based around pushing and pulling > commits, I think it does make sense. The author field may not always > match the committer (e.g., in a cherry-pick), but it still lets you > trace that attestation of the author back to the committer. And it's up > to UI to make that distinction clear (e.g., if you push a signed > cherry-pick to GitHub, the commit is labeled with something like "A U > Thor committed with C O Mitter", and then you get a little "Verified" > tag for C O Mitter that gives you more information about the signature). > > So I don't think it's just a checkbox feature. It's a useful thing for > certain workflows that really want to be able to attribute individual > commits with cryptographic strength. "certain workflows". :( See, I really like reading e.g. the "On Re-tagging" section of git-tag as it doesn't hand wave around the decisions to make. Now as a user I may already have a workflow that I like. And I might want to "bring in more security". Then I have to figure out possible attack scenarios and which sort of signing can prevent such an attack. And each organisation has to do that themselves, but we as the provider of the tool might have this knowledge because we implemented all these shiny "sign here, please" parts. Thanks for the lively discussion, Stefan
Re: Crash on MSYS2 with GIT_WORK_TREE
Hi valtron, On Tue, 7 Mar 2017, valtron wrote: > Git 1.12.0. I take it you meant 2.12.0. And you probably also meant to imply that you are referring to MSYS2's git.exe in /usr/bin/. > When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git > commands crash: > > C:\repo>set GIT_WORK_TREE=C:/repo > C:\repo>git rev-parse HEAD > 1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping > stack trace to git.exe.stackdump This suggests that my assumption above is correct: it looks as if you are executing \usr\bin\git.exe here. The proof lies in the pudding, though, and you are the only person who can do that unless you post the contents of that git.exe.stackdump. > Stacktrace from GDB (on git-rev-parse) is: > > #0 0x00018019634d in strcmp (s1=0x600062080 "/c/repo", s2=0x0) >at > /msys_scripts/msys2-runtime/src/msys2-runtime/newlib/libc/string/strcmp.c:83 > #1 0x0001005239f1 in ?? () > #2 0x000100523f36 in ?? () > #3 0x00010046c6fa in ?? () > #4 0x000100401b6d in ?? () > #5 0x000100401e4b in ?? () > #6 0x000100563593 in ?? () > #7 0x000180047c37 in _cygwin_exit_return () >at > /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/dcrt0.cc:1031 > #8 0x000180045873 in _cygtls::call2 (this=0xce00, > func=0x180046bd0 , arg=0x0, >buf=buf@entry=0xcdf0) at > /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:40 > #9 0x000180045924 in _cygtls::call (func=, > arg=) >at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:27 > #10 0x in ?? () > Backtrace stopped: previous frame inner to this frame (corrupt stack?) This may be the wrong thread, though. You can find out what other threads there are with `info threads` and switch by `thread `, the backtrace(s) of the other thread(s) may be informative. Please also note that installing the `msys2-runtime-devel` package via Pacman may be able to get you nicer symbols. In any case, this problem is squarely within the MSYS2 runtime. It has nothing to do with Git except for the motivation to set an environment variable to an absolute path as you outlined. Having said that, the problem also occurs when using *MSYS2* git.exe (i.e. /usr/bin/git.exe, not /mingw64/bin/git.exe) in the Git for Windows SDK. Ciao, Johannes
Re: GSoC 2017
Decided to rewrite module_clone to use connect_work_tree_and_git_dir as my microproject by suggestion of Stefan Bellar. https://public-inbox.org/git/cagz79ky+1e-wg0-uzgjme+haoe+1wcmg0eux7rwgtku_abd...@mail.gmail.com/ It seems that connect_work_tree_and_git_dir needs to be slightly changed to deal with submodules. Because path to config file for submodules lies not directly in .git directory we need to know whether the path is submodule or not. Can I use submodule_from_path to determine this? I mean what if NULL result also indicates error sometimes? Another possible solution is to pass the flag indicating that path is submodule or not. But I think this solution is bad. Regards, Valery Tolstov
Re: [PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.
On Tue, Mar 7, 2017 at 2:42 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> Signed-off-by: Stefan Beller
>> + submodule_move_head(sub->path, "HEAD", NULL, \
>
> What is this backslash doing here?
I am still bad at following coding conventions, apparently.
will remove.
>> @@ -532,8 +550,13 @@ void remove_marked_cache_entries(struct index_state
>> *istate)
>>
>> for (i = j = 0; i < istate->cache_nr; i++) {
>> if (ce_array[i]->ce_flags & CE_REMOVE) {
>> - remove_name_hash(istate, ce_array[i]);
>> - save_or_free_index_entry(istate, ce_array[i]);
>> + const struct submodule *sub =
>> submodule_from_ce(ce_array[i]);
>> + if (sub) {
>> + remove_submodule_according_to_strategy(sub);
>> + } else {
>> + remove_name_hash(istate, ce_array[i]);
>> + save_or_free_index_entry(istate, ce_array[i]);
>> + }
>
> I cannot readily tell as the proposed log message is on the sketchy
> side to explain the reasoning behind this, but this behaviour change
> is done unconditionally (in other words, without introducing a flag
> that is set by --recurse-submodules command line flag and switches
> behaviour based on the flag) here. What is the end-user visible
> effect with this change?
submodule_from_ce returns always NULL, when such flag is not given.
>From 10/18:
+const struct submodule *submodule_from_ce(const struct cache_entry *ce)
+{
+ if (!S_ISGITLINK(ce->ce_mode))
+ return NULL;
+
+ if (!should_update_submodules())
+ return NULL;
+
+ return submodule_from_path(null_sha1, ce->name);
+}
should_update_submodules is always false if such a flag is not set,
such that we end up in the else case which is literally the same as
the removed lines (they are just indented).
> Can we have a new test in this same patch
> to demonstrate the desired behaviour introduced by this change (or
> the bug this change fixes)?
This doesn't fix a bug, but in case the flag is given (in patches 17,18)
this new code removes submodules that are no longer recorded in
the tree.
Re: [PATCH v4 04/10] setup_git_directory_1(): avoid changing global state
On 03/07, Johannes Schindelin wrote:
> const char *setup_git_directory_gently(int *nongit_ok)
> {
> + struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, gitdir =
> STRBUF_INIT;
I couldn't see any strbuf_release() calls for these strbufs so there may
be some memory leaking here.
> const char *prefix;
>
> - prefix = setup_git_directory_gently_1(nongit_ok);
> + /*
> + * We may have read an incomplete configuration before
> + * setting-up the git directory. If so, clear the cache so
> + * that the next queries to the configuration reload complete
> + * configuration (including the per-repo config file that we
> + * ignored previously).
> + */
> + git_config_clear();
> +
> + /*
> + * Let's assume that we are in a git repository.
> + * If it turns out later that we are somewhere else, the value will be
> + * updated accordingly.
> + */
> + if (nongit_ok)
> + *nongit_ok = 0;
> +
> + if (strbuf_getcwd(&cwd))
> + die_errno(_("Unable to read current working directory"));
> + strbuf_addbuf(&dir, &cwd);
> +
> + switch (setup_git_directory_gently_1(&dir, &gitdir)) {
> + case GIT_DIR_NONE:
> + prefix = NULL;
> + break;
> + case GIT_DIR_EXPLICIT:
> + prefix = setup_explicit_git_dir(gitdir.buf, &cwd, nongit_ok);
> + break;
> + case GIT_DIR_DISCOVERED:
> + if (dir.len < cwd.len && chdir(dir.buf))
> + die(_("Cannot change to '%s'"), dir.buf);
> + prefix = setup_discovered_git_dir(gitdir.buf, &cwd, dir.len,
> + nongit_ok);
> + break;
> + case GIT_DIR_BARE:
> + if (dir.len < cwd.len && chdir(dir.buf))
> + die(_("Cannot change to '%s'"), dir.buf);
> + prefix = setup_bare_git_dir(&cwd, dir.len, nongit_ok);
> + break;
> + case GIT_DIR_HIT_CEILING:
> + prefix = setup_nongit(cwd.buf, nongit_ok);
> + break;
> + case GIT_DIR_HIT_MOUNT_POINT:
> + if (nongit_ok) {
> + *nongit_ok = 1;
> + return NULL;
> + }
> + die(_("Not a git repository (or any parent up to mount point
> %s)\n"
> + "Stopping at filesystem boundary
> (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> + dir.buf);
> + default:
> + die("BUG: unhandled setup_git_directory_1() result");
> + }
> +
> if (prefix)
> setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
> else
> --
> 2.12.0.windows.1.7.g94dafc3b124
>
>
--
Brandon Williams
Re: [PATCH 16/18] entry.c: update submodules when interesting
On Tue, Mar 7, 2017 at 3:04 PM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>>> +if (!is_submodule_populated_gently(ce->name, &err)) {
>>> +struct stat sb;
>>> +if (lstat(ce->name, &sb))
>>> +die(_("could not stat file '%s'"),
>>> ce->name);
>>> +if (!(st.st_mode & S_IFDIR))
>>> +unlink_or_warn(ce->name);
>>
>> We found that the path ce->name is supposed to be a submodule that
>> is checked out, we found that the submodule is not yet populated, we
>> tried to make sure what is on that path, and its failure would cause
>> us to die(). All that is sensible.
>> ...
>> But if that unlink fails, shouldn't we die, just like the case where
>> we cannot tell what is at the path ce->name?
>>
>> And if that unlink succeeds, instead of having an empty directory,
>> we start the "move-head" call to switch from NULL to ce->oid without
>> having any directory. Wouldn't we want to mkdir() here (and remove
>> mkdir() in submodule_move_head() if there is one---if there isn't
>> then I do not think this codepath would work)?
>
> In addition to mkdir(), would we also need the .git file that point
> into superproject's .git/modules/ too?
The move_head function takes care of it
Both creation as well as deletion are handled in the move_head function,
when either new or old is NULL.
We can drag that out of that function and have it at the appropriate places
manually.
Why do you think it is better design to have mkdir here and not in move_head?
(For me having everything in move_head was easier to understand,
maybe it is not for others)
Thanks,
Stefan
Re: [PATCH v4 04/10] setup_git_directory_1(): avoid changing global state
Johannes Schindelin writes:
> + switch (setup_git_directory_gently_1(&dir, &gitdir)) {
> + case GIT_DIR_NONE:
> + prefix = NULL;
> + break;
> + case GIT_DIR_EXPLICIT:
> + prefix = setup_explicit_git_dir(gitdir.buf, &cwd, nongit_ok);
> + break;
> + case GIT_DIR_DISCOVERED:
> + if (dir.len < cwd.len && chdir(dir.buf))
> + die(_("Cannot change to '%s'"), dir.buf);
> + prefix = setup_discovered_git_dir(gitdir.buf, &cwd, dir.len,
> + nongit_ok);
> + break;
> + case GIT_DIR_BARE:
> + if (dir.len < cwd.len && chdir(dir.buf))
> + die(_("Cannot change to '%s'"), dir.buf);
> + prefix = setup_bare_git_dir(&cwd, dir.len, nongit_ok);
> + break;
> + case GIT_DIR_HIT_CEILING:
> + prefix = setup_nongit(cwd.buf, nongit_ok);
> + break;
> + case GIT_DIR_HIT_MOUNT_POINT:
> + if (nongit_ok) {
> + *nongit_ok = 1;
> + return NULL;
> + }
> + die(_("Not a git repository (or any parent up to mount point
> %s)\n"
> + "Stopping at filesystem boundary
> (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> + dir.buf);
> + default:
> + die("BUG: unhandled setup_git_directory_1() result");
> + }
I _might_ find niggles in other patches (and other parts of this
patch) that enables the above clean implementation, but this
switch() statement speaks of the value of this entire series ;-)
Very nicely done.
Re: [PATCH 16/18] entry.c: update submodules when interesting
Junio C Hamano writes:
>> +if (!is_submodule_populated_gently(ce->name, &err)) {
>> +struct stat sb;
>> +if (lstat(ce->name, &sb))
>> +die(_("could not stat file '%s'"),
>> ce->name);
>> +if (!(st.st_mode & S_IFDIR))
>> +unlink_or_warn(ce->name);
>
> We found that the path ce->name is supposed to be a submodule that
> is checked out, we found that the submodule is not yet populated, we
> tried to make sure what is on that path, and its failure would cause
> us to die(). All that is sensible.
> ...
> But if that unlink fails, shouldn't we die, just like the case where
> we cannot tell what is at the path ce->name?
>
> And if that unlink succeeds, instead of having an empty directory,
> we start the "move-head" call to switch from NULL to ce->oid without
> having any directory. Wouldn't we want to mkdir() here (and remove
> mkdir() in submodule_move_head() if there is one---if there isn't
> then I do not think this codepath would work)?
In addition to mkdir(), would we also need the .git file that point
into superproject's .git/modules/ too?
Re: fatal error when diffing changed symlinks
Hi Junio,
On Tue, 7 Mar 2017, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> >> > When viewing a working tree file, oid.hash could be 0{40} and
> >> > read_sha1_file() is not the right function to use to obtain the
> >> > contents.
> >> >
> >> > Both of these two need to pay attention to 0{40}, I think, as the
> >> > user may be running "difftool -R --dir-diff" in which case the
> >> > working tree would appear in the left hand side instead.
> >>
> >> As a side note, I think even outside of 0{40}, this should be checking
> >> the return value of read_sha1_file(). A corrupted repo should die(), not
> >> segfault.
> >
> > I agree. I am on it.
>
> Friendly ping, if only to make sure that we can keep a piece of this
> thread in the more "recent" pile.
>
> If you have other topics you need to perfect, I think it is OK to
> postpone the fix on this topic a bit longer, but I'd hate to ship
> two releases with a known breakage without an attempt to fix it, so
> if you are otherwise occupied, I may encourage others (including me)
> to take a look at this. The new "difftool" also has a reported
> regression somebody else expressed willingness to work on, which is
> sort of blocked by everybody else not knowing the timeline on this
> one. cf. <20170303212836.GB13790@arch-attack.localdomain>
>
> A patch series would be very welcome, but "Please go ahead if
> somebody else has time, and I'll help reviewing" would be also
> good.
Unfortunately, the obvious fix was not enough. My current work in progress
is in the `difftool-null-sha1` branch on https://gihub.com/dscho/git. I
still have a few other things to tend to, but hope to come back to the
difftool before the end of the week.
Ciao,
Johannes
Re: [PATCH 4/6] send-pack: improve unpack-status error messages
Jeff King writes:
> When the remote tells us that the "unpack" step failed, we
> show an error message. However, unless you are familiar with
> the internals of send-pack and receive-pack, it was not
> clear that this represented an error on the remote side.
> Let's re-word to make that more obvious.
>
> Likewise, when we got an unexpected packet from the other
> end, we complained with a vague message but did not actually
> show the packet. Let's fix that.
Both make sense.
> And finally, neither message was marked for translation. The
> message from the remote probably won't be translated, but
> there's no reason we can't do better for the local half.
Hmm, OK.
>
> Signed-off-by: Jeff King
> ---
> send-pack.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/send-pack.c b/send-pack.c
> index 243633da1..83c23aef6 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -134,9 +134,9 @@ static int receive_unpack_status(int in)
> {
> const char *line = packet_read_line(in, NULL);
> if (!skip_prefix(line, "unpack ", &line))
> - return error("did not receive remote status");
> + return error(_("unable to parse remote unpack status: %s"),
> line);
> if (strcmp(line, "ok"))
> - return error("unpack failed: %s", line);
> + return error(_("remote unpack failed: %s"), line);
> return 0;
> }
Re: [PULL] git svn branch authentication fix
Eric Wong writes: > Thank you. I fixed spelling in the title (s/authenticaton/authentication/), > added my S-o-b, and pushed for Junio to pick up > > The following changes since commit 3bc53220cb2dcf709f7a027a3f526befd021d858: > > First batch after 2.12 (2017-02-27 14:04:24 -0800) > > are available in the git repository at: > > git://bogomips.org/git-svn.git svn-auth-branch > > for you to fetch changes up to e0688e9b28f2c5ff711460ee8b62077be5df2360: > > git svn: fix authentication with 'branch' (2017-03-07 21:29:03 +) > > > Hiroshi Shirosaki (1): > git svn: fix authentication with 'branch' > > git-svn.perl | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Thanks, both. Will pull.
Re: [PATCH 05/18] lib-submodule-update.sh: define tests for recursing into submodules
Stefan Beller writes: > + # ... and ignored files are ignroed ignored.
Re: [PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.
Stefan Beller writes:
> Signed-off-by: Stefan Beller
> ---
> read-cache.c | 27 +--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 9054369dd0..9a2abacf7a 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -18,6 +18,8 @@
> #include "varint.h"
> #include "split-index.h"
> #include "utf8.h"
> +#include "submodule.h"
> +#include "submodule-config.h"
>
> /* Mask for the name length in ce_flags in the on-disk index */
>
> @@ -520,6 +522,22 @@ int remove_index_entry_at(struct index_state *istate,
> int pos)
> return 1;
> }
>
> +static void remove_submodule_according_to_strategy(const struct submodule
> *sub)
> +{
> + switch (sub->update_strategy.type) {
> + case SM_UPDATE_UNSPECIFIED:
> + case SM_UPDATE_CHECKOUT:
> + case SM_UPDATE_REBASE:
> + case SM_UPDATE_MERGE:
> + submodule_move_head(sub->path, "HEAD", NULL, \
What is this backslash doing here?
> + SUBMODULE_MOVE_HEAD_FORCE);
> + break;
> + case SM_UPDATE_NONE:
> + case SM_UPDATE_COMMAND:
> + ; /* Do not touch the submodule. */
> + }
> +}
> +
> /*
> * Remove all cache entries marked for removal, that is where
> * CE_REMOVE is set in ce_flags. This is much more effective than
> @@ -532,8 +550,13 @@ void remove_marked_cache_entries(struct index_state
> *istate)
>
> for (i = j = 0; i < istate->cache_nr; i++) {
> if (ce_array[i]->ce_flags & CE_REMOVE) {
> - remove_name_hash(istate, ce_array[i]);
> - save_or_free_index_entry(istate, ce_array[i]);
> + const struct submodule *sub =
> submodule_from_ce(ce_array[i]);
> + if (sub) {
> + remove_submodule_according_to_strategy(sub);
> + } else {
> + remove_name_hash(istate, ce_array[i]);
> + save_or_free_index_entry(istate, ce_array[i]);
> + }
I cannot readily tell as the proposed log message is on the sketchy
side to explain the reasoning behind this, but this behaviour change
is done unconditionally (in other words, without introducing a flag
that is set by --recurse-submodules command line flag and switches
behaviour based on the flag) here. What is the end-user visible
effect with this change? Can we have a new test in this same patch
to demonstrate the desired behaviour introduced by this change (or
the bug this change fixes)?
> }
> else
> ce_array[j++] = ce_array[i];
Re: [PATCH 18/18] builtin/read-tree: add --recurse-submodules switch
Stefan Beller writes: > A new known failure mode is introduced[1], which is actually not > a failure but a feature in read-tree. Unlike checkout for which > the recursive submodule tests were originally written, read-tree does > warn about ignored untracked files that would be overwritten. > For the sake of keeping the test library for submodules genric, just generic
Re: [PATCH 16/18] entry.c: update submodules when interesting
Stefan Beller writes:
> Signed-off-by: Stefan Beller
> ---
> entry.c | 30 ++
> 1 file changed, 30 insertions(+)
>
> diff --git a/entry.c b/entry.c
> index c6eea240b6..d2b512da90 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -2,6 +2,7 @@
> #include "blob.h"
> #include "dir.h"
> #include "streaming.h"
> +#include "submodule.h"
>
> static void create_directories(const char *path, int path_len,
> const struct checkout *state)
> @@ -146,6 +147,7 @@ static int write_entry(struct cache_entry *ce,
> unsigned long size;
> size_t wrote, newsize = 0;
> struct stat st;
> + const struct submodule *sub;
>
> if (ce_mode_s_ifmt == S_IFREG) {
> struct stream_filter *filter = get_stream_filter(ce->name,
> @@ -203,6 +205,10 @@ static int write_entry(struct cache_entry *ce,
> return error("cannot create temporary submodule %s",
> path);
> if (mkdir(path, 0777) < 0)
> return error("cannot create submodule directory %s",
> path);
> + sub = submodule_from_ce(ce);
> + if (sub)
> + return submodule_move_head(ce->name,
> + NULL, oid_to_hex(&ce->oid),
> SUBMODULE_MOVE_HEAD_FORCE);
> break;
> default:
> return error("unknown file mode for %s in index", path);
> @@ -259,7 +265,31 @@ int checkout_entry(struct cache_entry *ce,
> strbuf_add(&path, ce->name, ce_namelen(ce));
>
> if (!check_path(path.buf, path.len, &st, state->base_dir_len)) {
> + const struct submodule *sub;
> unsigned changed = ce_match_stat(ce, &st,
> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);
> + /*
> + * Needs to be checked before !changed returns early,
> + * as the possibly empty directory was not changed
> + */
> + sub = submodule_from_ce(ce);
> + if (sub) {
> + int err;
> + if (!is_submodule_populated_gently(ce->name, &err)) {
> + struct stat sb;
> + if (lstat(ce->name, &sb))
> + die(_("could not stat file '%s'"),
> ce->name);
> + if (!(st.st_mode & S_IFDIR))
> + unlink_or_warn(ce->name);
We found that the path ce->name is supposed to be a submodule that
is checked out, we found that the submodule is not yet populated, we
tried to make sure what is on that path, and its failure would cause
us to die(). All that is sensible.
Then we want to make sure the filesystem entity at the path
currently is a directory and otherwise unlink (i.e. we may have an
unrelated file that is not tracked there, or perhaps the user earlier
decided that replacing the submodule with a single file is a good
idea, but then getting rid of the change with "checkout -f" before
doing "git add" on that file). That is also sensible.
But if that unlink fails, shouldn't we die, just like the case where
we cannot tell what is at the path ce->name?
And if that unlink succeeds, instead of having an empty directory,
we start the "move-head" call to switch from NULL to ce->oid without
having any directory. Wouldn't we want to mkdir() here (and remove
mkdir() in submodule_move_head() if there is one---if there isn't
then I do not think this codepath would work)?
If we had a directory here, but if that directory is not empty,
should we proceed? I am assuming (I haven't carefully read
"move-head") that it is OK because the "run an equivalent of
'checkout --detach ce->oid'" done by "move-head" would notice a
possible information loss to overwrite an untracked path (everything
is "untracked" as the head moves from NULL to ce->oid in this case),
and prevent it from happening.
Am I reading the design correctly?
> + return submodule_move_head(ce->name,
> + NULL, oid_to_hex(&ce->oid),
> + SUBMODULE_MOVE_HEAD_FORCE);
> + } else
> + return submodule_move_head(ce->name,
> + "HEAD", oid_to_hex(&ce->oid),
> + SUBMODULE_MOVE_HEAD_FORCE);
> + }
> +
> if (!changed)
> return 0;
> if (!state->force) {
[PULL] git svn branch authentication fix
Thank you. I fixed spelling in the title (s/authenticaton/authentication/), added my S-o-b, and pushed for Junio to pick up The following changes since commit 3bc53220cb2dcf709f7a027a3f526befd021d858: First batch after 2.12 (2017-02-27 14:04:24 -0800) are available in the git repository at: git://bogomips.org/git-svn.git svn-auth-branch for you to fetch changes up to e0688e9b28f2c5ff711460ee8b62077be5df2360: git svn: fix authentication with 'branch' (2017-03-07 21:29:03 +) Hiroshi Shirosaki (1): git svn: fix authentication with 'branch' git-svn.perl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Crash on MSYS2 with GIT_WORK_TREE
Git 1.12.0. When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git commands crash: C:\repo>set GIT_WORK_TREE=C:/repo C:\repo>git rev-parse HEAD 1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping stack trace to git.exe.stackdump C:\repo>set GIT_WORK_TREE= C:\repo>git rev-parse HEAD a394e40861e1012a96f9578a1f0cf0c5a49ede11 On the other hand, "C:\repo" and "/c/repo" don't have this issue. Stacktrace from GDB (on git-rev-parse) is: #0 0x00018019634d in strcmp (s1=0x600062080 "/c/repo", s2=0x0) at /msys_scripts/msys2-runtime/src/msys2-runtime/newlib/libc/string/strcmp.c:83 #1 0x0001005239f1 in ?? () #2 0x000100523f36 in ?? () #3 0x00010046c6fa in ?? () #4 0x000100401b6d in ?? () #5 0x000100401e4b in ?? () #6 0x000100563593 in ?? () #7 0x000180047c37 in _cygwin_exit_return () at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/dcrt0.cc:1031 #8 0x000180045873 in _cygtls::call2 (this=0xce00, func=0x180046bd0 , arg=0x0, buf=buf@entry=0xcdf0) at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:40 #9 0x000180045924 in _cygtls::call (func=, arg=) at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:27 #10 0x in ?? () Backtrace stopped: previous frame inner to this frame (corrupt stack?) It seems "C:/repo" was changed to "/c/repo", but it crashes because it gets compared to a nullptr.
Re: [PATCH] t*: avoid using pipes
On Tue, Mar 7, 2017 at 12:39 PM, Johannes Sixt wrote: > Welcome to the Git community! > > Actually, being a *micro* project, it should stay so. Not doing all of the > changes would leave some tasks for other apprentices to get warm with our > review process. right, so just pick one file. > Thank you, Stefan, for digging out one particularly interesting case. > >> When looking at the content, the conversion seems a bit mechanical >> (which is fine for a micro project), such as: >> ... >> - test "$(git show --pretty=format:%s | head -n 1)" = "one" >> + test "$(git show --pretty=format:%s >out && head -n 1 > ... >> >> specifically for the "head" command I don't think it makes a >> difference in correctness whether you pipe the file into the tool >> or give the filename, i.e. "head -n 1 out" would work just as fine. > > > True, but! > > The intent of removing git invocations from the upstream of a pipe is that a > failure exit code is able to stop a && chain. Doh! I was so fixated over discussing whether to use "<" or not, to miss looking for the actual goal. Thanks for spotting! Stefan
Re: [PATCH] t*: avoid using pipes
Am 07.03.2017 um 18:21 schrieb Stefan Beller: On Tue, Mar 7, 2017 at 8:10 AM, Prathamesh Chavan wrote: I'm Prathamesh Chavan. As a part of my micropraoject I have been working on "Avoid pipes for git related commands in test suites". Welcome to the Git community! * keep it micro first, e.g. just convert one file, send to list, wait for reviewers feedback and incorporate that (optional step after having done the full development cycle: convert all the other files; each as its own patch) * split up this one patch into multiple patches, e.g. one file per patch, then send a patch series. Actually, being a *micro* project, it should stay so. Not doing all of the changes would leave some tasks for other apprentices to get warm with our review process. https://github.com/pratham-pc/git/tree/micro-proj While I did look at that, not everyone here in the git community does so. (Also for getting your change in, Junio seems to strongly prefer patches on list instead of e.g. fetching and cherry-picking from your github) Thank you, Stefan, for digging out one particularly interesting case. When looking at the content, the conversion seems a bit mechanical (which is fine for a micro project), such as: ... - test "$(git show --pretty=format:%s | head -n 1)" = "one" + test "$(git show --pretty=format:%s >out && head -n 1 True, but! The intent of removing git invocations from the upstream of a pipe is that a failure exit code is able to stop a && chain. The example above does not achieve this even after removal of the pipe. The reason is that exit code of process expansions $(...) is usually ignored. To meet the intent, further changes are necessary, for example to: git show --pretty=format:%s >out && test "$(head -n 1 out)" = "one" Side note: There is one exception where the exit code of $(...) is not ignored: when it occurs in the last variable assignment of a command that consists only of variable assignments. -- Hannes
Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree
On Tue, Mar 7, 2017 at 10:44 AM, Junio C Hamano wrote:
> So perhaps your superproject_exists() helper can be eliminated
That is what I had originally, but I assumed a strict helper function
for "existence of the superproject" would be interesting in the future,
e.g. for get_superproject_git_dir, or on its own. There was an attempt
to have the shell prompt indicate if you are in a submodule,
which would not need to know the worktree or git dir of the
superproject, but only its existence.
> instead coded in get_superproject_working_tree() in place to do:
>
> - xgetcwd() to get "/local/repo/super/sub/dir".
Did you mean .../super/dir/sub ?
If not and we run this command from a directory inside the
submodule, the usual prefix mechanics should go to the
root of the submodule, such that the ".." will be just enough
to break out of that submodule repo.
The interesting part is in the superproject, to see if we are
in a directory (and where the root of the superproject is).
> - relative_path() to get "dir".
ok.
> - ask "ls-{tree,files} --full-name HEAD dir" to get "16"
> and "sub/dir".
"ls-files --stage --full-name" to get
16 ... dir/sub
>
> - subtract "sub/dir" from the tail of the "/local/repo/super/sub/dir"
> you got from xgetcwd() earlier.
makes sense.
>
> - return the result.
>
> with a failure/unmet expectations (like not finding 16) from any
> step returning an error, or something like that.
That seems better as we only need to spawn one process.
(This could also mean I am bad at reading our own man pages)
Thanks,
Stefan
Re: Reg : GSoC 2017 Microproject
Hi, On Tue, Mar 7, 2017 at 11:22 AM, Vedant Bassi wrote: > Hi, > > I would like to participate in GSoC 2017 and I have chosen the Use > unsigned integral type for collection of bits , idea from the Micro > projects list. > > I request the help of the community for clarifying a few questions that I > have. > > 1. Is this Microproject already taken ? As already suggested, it is a good idea to search the mailing list. This way you can find if it has already been taken or discussed. You can use https://public-inbox.org/git/ for that. > 2. If it is free , I would like to point out one place where a signed > int is used . > > In bisect.h , the structure struct rev_list_info uses flags of > type signed int but , the value of MSB is not checked as a test case > for any error checking. Hence it can be of type unsigned int. > It is only used in rev-list.c for checking cases (BISECT_SHOW_ALL and > REV_LIST_QUIET ). > > Is this a valid case. Yeah, it looks like it is a valid case.
Re: [PATCH] repack: Add options to preserve and prune old pack files
James Melvin writes:
> These options are designed to prevent stale file handle exceptions
> during git operations which can happen on users of NFS repos when
> repacking is done on them. The strategy is to preserve old pack files
> around until the next repack with the hopes that they will become
> unreferenced by then and not cause any exceptions to running processes
> when they are finally deleted (pruned).
I find it a very sensible strategy to work around NFS, but it does
not explain why the directory the old ones are moved to need to be
configurable. It feels to me that a boolean that causes the old
ones renamed s/^pack-/^old-&/ in the same directory (instead of
pruning them right away) would risk less chances of mistakes (e.g.
making "preserved" subdirectory on a separate device mounted there
in a hope to reduce disk usage of the primary repository, which
may defeat the whole point of moving the still-active file around
instead of removing them).
And if we make "preserve-old" a boolean, perhaps the presence of
"prune-preserved" would serve as a substitute for it, iow, perhaps
we may only need --prune-preserved option (and repack.prunePreserved
configuration variable)?
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 677bc7c81..f1a0c97f3 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -10,8 +10,10 @@
>
> static int delta_base_offset = 1;
> static int pack_kept_objects = -1;
> +static int preserve_oldpacks = 0;
> +static int prune_preserved = 0;
We avoid initializing statics to 0 or NULL and instead let BSS take
care of them...
> static int write_bitmaps;
> -static char *packdir, *packtmp;
> +static char *packdir, *packtmp, *preservedir;
... just like what you did here.
> @@ -108,6 +110,27 @@ static void get_non_kept_pack_filenames(struct s
> ...
> +static void preserve_pack(const char *file_path, const char *file_name,
> const char *file_ext)
> +{
> + char *fname_old;
> +
> + if (mkdir(preservedir, 0700) && errno != EEXIST)
> + error(_("failed to create preserve directory"));
You do not want to do the rest of this function after issuing this
error, no? Because ...
> +
> + fname_old = mkpathdup("%s/%s.old-%s", preservedir, file_name,
> ++file_ext);
> + rename(file_path, fname_old);
... this rename(2) would fail, whose error return you would catch
and act on.
> + free(fname_old);
> +}
> +
> +static void remove_preserved_dir(void) {
> + struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_addstr(&buf, preservedir);
> + remove_dir_recursively(&buf, 0);
This is a wrong helper function to use on files and directories
inside .git/; the function is about removing paths in the working
tree.
> @@ -121,7 +144,10 @@ static void remove_redundant_pack(const char *dir_name,
> const char *base_name)
> for (i = 0; i < ARRAY_SIZE(exts); i++) {
> strbuf_setlen(&buf, plen);
> strbuf_addstr(&buf, exts[i]);
> - unlink(buf.buf);
> + if (preserve_oldpacks)
> + preserve_pack(buf.buf, base_name, exts[i]);
> + else
> + unlink(buf.buf);
OK.
> @@ -194,6 +220,10 @@ int cmd_repack(int argc, const char **argv, const char
> *prefix)
> N_("maximum size of each packfile")),
> OPT_BOOL(0, "pack-kept-objects", &pack_kept_objects,
> N_("repack objects in packs marked with
> .keep")),
> + OPT_BOOL(0, "preserve-oldpacks", &preserve_oldpacks,
> + N_("move old pack files into the preserved
> subdirectory")),
> + OPT_BOOL(0, "prune-preserved", &prune_preserved,
> + N_("prune old pack files from the preserved
> subdirectory after repacking")),
> OPT_END()
> };
>
> @@ -217,6 +247,7 @@ int cmd_repack(int argc, const char **argv, const char
> *prefix)
>
> packdir = mkpathdup("%s/pack", get_object_directory());
> packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
> + preservedir = mkpathdup("%s/preserved", packdir);
>
> sigchain_push_common(remove_pack_on_signal);
>
> @@ -404,6 +435,9 @@ int cmd_repack(int argc, const char **argv, const char
> *prefix)
>
> /* End of pack replacement. */
>
> + if (prune_preserved)
> + remove_preserved_dir();
I am not sure if I understand your design. Your model looks to me
like there are two modes of operation. #1 uses "--preserve-old" and
sends old ones to purgatory instead of removing them and #2 uses
"--prune-preserved" to remove all the ones in the purgatory
immediately. A few things that come to my mind immediately:
* When "--prune-preseved" is used, it removes both ancient ones and
more recent ones indiscriminately. Would it make more sense to
"expire" only the older ones while keeping the more recent ones?
* It appears that the main reason you would want
Re: [RESEND PATCH] git-gui--askpass: generalize the window title
On Tue, Mar 7, 2017 at 7:30 PM, Stefan Beller wrote: > Although the following are included in git.git repository, they have their > own authoritative repository and maintainers: Thanks. I continuously get confused by this fact. -- Sebastian Schuberth
Re: git init --separate-git-dir does not update symbolic .git links for submodules
On Tue, Mar 7, 2017 at 11:52 AM, Valery Tolstov wrote: > Just noticed that there already is function that gives module list > module_list_compute. But purpose of it's arguments is not quite clear > for me at this moment. > static int module_list_compute(int argc, const char **argv, const char *prefix, struct pathspec *pathspec, struct module_list *list) argc, argv and prefix are just passed through from each caller argc is the number of arguments on the command line, argv is an array of said arguments on the command line, prefix is the position inside the repository. e.g. If in git.git in Documentation/, you run a git command "git submodule--helper module_list *" then first "git" and "submodule--helper" are removed, and then cmd_submodule__helper is called (at the end of the submodule helper, which then sees argc=2, argv=["module_list", "*"], prefix="Documentation/" which then proceeds to into module_list(). That parses these arguments, but there is no argument it knows about, so it does nothing. Then it just passes these three (argc, argv, prefix) to module_list_compute, which then makes up the list, to be stored in the last parameter 'list'. The 'pathspec' parameter seems weird. Internally the arguments mentioned above are converted to a pathspec, such that we can go through all files and call match_pathspec inside of module_list_compute. In all but one cases we do not care about the pathspec, but in one case (in update_clone) we want to issue a warning if the pathspec was empty (i.e. the user just said "git submodule--helper update_clone" with no further path arguments)
Re: RFC: Another proposed hash function transition plan
Jonathan Nieder writes ("RFC: Another proposed hash function transition plan"):
> This past week we came up with this idea for what a transition to a new
> hash function for Git would look like. I'd be interested in your
> thoughts (especially if you can make them as comments on the document,
> which makes it easier to address them and update the document).
Thanks for this.
This is a reasonable plan. It corresponds to approaches (2) and (B)
of my survey mail from the other day. Ie, two parallel homogeneous
hash trees, rather than a unified but heterogeneous hash tree, with
old vs new object names distinguished by length.
I still prefer my proposal with the mixed hash tree, mostly because
the handling of signatures here is very awkward, and because my
proposal does not involve altering object ids stored other than in the
git object graph (eg CI system databases, etc.)
One thing you've missed, I think, is notes: notes have to be dealt
with in a more complicated way. Do you intend to rewrite the tree
objects for notes commits so that the notes are annotations for the
new names for the annotated objects ? And if so, when ?
Also I think you need to specify how abbreviated object names are
interpreted.
Regards,
Ian.
STRICTLY CONFIDENTIAL
I have important transaction for you as next of kin to claim US$18.37m Mail me on my private email: chimwia...@gmail.com so I can send you more details Thanks Mr.Chim Wai Kim MOVE TO INBOX=== DISCLAIMER: This email and any files it contains are confidential and intended for the use of the recipient(s) only. If you are not the intended recipient you should notify the sender immediately and destroy the material from your system.
Re: git init --separate-git-dir does not update symbolic .git links for submodules
Just noticed that there already is function that gives module list module_list_compute. But purpose of it's arguments is not quite clear for me at this moment.
Re: git init --separate-git-dir does not update symbolic .git links for submodules
On Tue, Mar 7, 2017 at 10:59 AM, Valery Tolstov wrote:
> I think we can reuse code from module_clone that writes .git link.
> Possibly this code fragment needs to be factored out from module_clone
That fragment already exists, see dir.h:
connect_work_tree_and_git_dir(work_tree, git_dir);
Maybe another good microproject is to use that in module_clone.
>
> Also, to fix all the links, we need to obtain the list of submodules
> and then iterate over them.
Right, but a submodule may have a nested submodule.
So we need to fix each submodule from that list recursively,
i.e. not just the submodule itself, but any potential nested
submodule in that submodule, too.
(the listing doesn't list these nested submodules)
So we would call
fix_gitlink(sub)
{
fix_locally(sub);
// have a child process that calls
// this function on any submodule inside sub.
}
> module_list command iterates
> and prints the result to stdout. Maybe we can reuse this output.
> Or create separate function that returns the list of submodules.
yeah you can make use of module_list_compute to just produce
the list internally.
>
> Can we call these functions from submodule--helper inside init command,
> or run them thorugh internal command interface?
Both sounds fine, though calling internally is preferable for
performance reasons.
>
> In my opinion, make submodule--helper fully responsible for link fixes
> would be a good solution. Then we create two additional function, one
> that fixes all submodules on the current level, and another that
> fixes individual submodule.
> Although it looks good, I'm not quite sure that it's really good.
That sounds good to me.
So "git init --separate-git-dir" calls internally a new function
in the submodule--helper to
>
> So, maybe we can do link fixes like this:
>
>1. Start fixing from init command using submodule--helper, with
> subcommand that fixes all submodules on current level
for this step we do not change the repository we are in, so there
is no need to call a new process, but we rather want to
call it internally.
>2. Each submodule processed with another subcommand/function in
> submodule--helper individually
sounds good.
>3. Repeat for current submodule recursively
and this recursive action needs to have its own process in the submodule,
e.g. "git submodule--helper --recursive fix-git-links"
>
> Glad to see your advices.
>
> Regards,
> Valery Tolstov
Regards,
Stefan
Re: RFC: Another proposed hash function transition plan
On Tue, Mar 7, 2017 at 10:57 AM, Ian Jackson wrote: > > Also I think you need to specify how abbreviated object names are > interpreted. One option might be to not use hex for the new hash, but base64 encoding. That would make the full size ASCII hash encoding length roughly similar (43 base64 characters rather than 40), which would offset some of the new costs (longer filenames in the loose format, for example). Also, since 256 isn't evenly divisible by 6, and because you'd want some way to explictly disambiguate the new hashes, the rule *could* be that the ASCII representation of a new hash is the base64 encoding of the 258-bit value that has "10" prepended to it as padding. That way the first character of the hash would be guaranteed to not be a hex digit, because it would be in the range [g-v] (indexes 32..47). Of course, the downside is that base64 encoded hashes can also end up looking very much like real words, and now case would matter too. The "use base64 with a "10" two-bit padding prepended" also means that the natural loose format radix format would remain the first 2 characters of the hash, but due to the first character containing the padding, it would be a fan-out of 2**10 rather than 2**12. Of course, having written that, I now realize how it would cause problems for the usual shit-for-brains case-insensitive filesystems. So I guess base64 encoding doesn't work well for that reason. Linus
Guarantee Deal.
I am Capt. Haggard I have special proposal that will do you good, contact me for more understanding. Regard, Capt Haggard nathaniel Email: haggardnathanie...@gmail.com
Re: git init --separate-git-dir does not update symbolic .git links for submodules
I think we can reuse code from module_clone that writes .git link. Possibly this code fragment needs to be factored out from module_clone Also, to fix all the links, we need to obtain the list of submodules and then iterate over them. module_list command iterates and prints the result to stdout. Maybe we can reuse this output. Or create separate function that returns the list of submodules. Can we call these functions from submodule--helper inside init command, or run them thorugh internal command interface? In my opinion, make submodule--helper fully responsible for link fixes would be a good solution. Then we create two additional function, one that fixes all submodules on the current level, and another that fixes individual submodule. Although it looks good, I'm not quite sure that it's really good. So, maybe we can do link fixes like this: 1. Start fixing from init command using submodule--helper, with subcommand that fixes all submodules on current level 2. Each submodule processed with another subcommand/function in submodule--helper individually 3. Repeat for current submodule recursively Glad to see your advices. Regards, Valery Tolstov
Re: [PATCH v5 1/1] config: add conditional include
On Tue, Mar 7, 2017 at 12:47 AM, Jeff King wrote:
> On Mon, Mar 06, 2017 at 02:44:27PM -0800, Stefan Beller wrote:
>
>> > +static int include_condition_is_true(const char *cond, size_t cond_len)
>> > +{
>> ...
>> > +
>> > + error(_("unrecognized include condition: %.*s"), (int)cond_len,
>> > cond);
>> > + /* unknown conditionals are always false */
>> > + return 0;
>> > +}
>>
>> Thanks for putting an error message here. I was looking at what
>> is currently queued as origin/nd/conditional-config-include,
>> which doesn't have this error() (yet / not any more?)
>
> It's "not any more". It was in the original and I asked for it to be
> removed during the last review.
Okay. The joys of contradicting opinions on a mailing list. :)
>
>> I'd strongly suggest to keep the error message here as that way
>> a user can diagnose e.g. a typo in the condition easily.
>>
>> If we plan to extend this list of conditions in the future, and a user
>> switches between versions of git, then they may see this message
>> on a regular basis (whenever they use the 'old' version).
>
> That would make it unlike the rest of the config-include mechanism
> (which quietly ignores things it doesn't understand, like include.foo,
> or include.foo.path), as well as the config code in general (which
> ignores misspelt keys).
>
> Some of that "quiet when you don't understand it" is historical
> necessity. Older versions _can't_ complain about not knowing
> include.path, because they don't yet know it's worth complaining about.
agreed
> Likewise here, if this ships in v2.13 and a new condition "foo:" ships
> in v2.14, you get:
>
> v2.12 - no complaint; we don't even understand includeIf at all
> v2.13 - complain; we know includeIf, but not "foo:"
> v2.14 - works as expected
>
> Which is kind of weird and inconsistent. But maybe the typo-detection
> case is more important to get right than consistency across historical
> versions.
Oh, I see. I was contemplating a future in which 2.12 is not used anymore.
When looking at other examples, such as url.<...>.insteadOf we also do not
warn about typos (well we can't actually).
In diff..(command/binary/..) we know the limited set of drivers,
which is similar to the situation we have here.
Maybe a compromise between typo checking (edit distance < 2 -> warn;
silent for larger distances) and the consistency over time is desired.
But this is even more code to write.
So for now I retract my strong opinion and be happy with what is
presented as-is for the reasons Peff gave.
Thanks,
Stefan
Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree
Stefan Beller writes:
> +const char *get_superproject_working_tree()
const char *get_superproject_working_tree(void)
The same for the *.h file declaration.
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf sb = STRBUF_INIT;
> +
> + if (!superproject_exists())
> + return NULL;
> + ...
> + return strbuf_detach(&sb, NULL);
Having reviewed it, I somehow think you do not want to have a
separate superproject_exists() that grabs some part of the
information this caller needs and then discards it.
The helper already does these things:
- xgetcwd(), which may give you "/local/repo/super/sub/dir"
- relative_path() with the result and "..", which may give you
"dir"
- ls-tree HEAD "dir" to see what is in "sub/dir" of the
repository that governs ".."; if "sub/dir" is a gitlink,
you know you started in a working tree of a repository
different from the one that governs "..".
And the caller is trying to figure out where the root of the
superproject is, i.e. "/local/repo/super", and the helper has half
of the answer to that already. If you ask the "ls-tree HEAD" (as I
said, I think it should be "ls-files") to give you not "dir" but
"sub/dir", you can subtract it from the result of xgetcwd() you did
at the beginning of the helper, and that gives what this caller of
the helper wants.
So perhaps your superproject_exists() helper can be eliminated and
instead coded in get_superproject_working_tree() in place to do:
- xgetcwd() to get "/local/repo/super/sub/dir".
- relative_path() to get "dir".
- ask "ls-{tree,files} --full-name HEAD dir" to get "16"
and "sub/dir".
- subtract "sub/dir" from the tail of the "/local/repo/super/sub/dir"
you got from xgetcwd() earlier.
- return the result.
with a failure/unmet expectations (like not finding 16) from any
step returning an error, or something like that.
[PATCH 3/6] send-pack: use skip_prefix for parsing unpack status
This avoids repeating ourselves, and the use of magic
numbers.
Signed-off-by: Jeff King
---
Obviously not necessary, but just a cleanup while I was here.
send-pack.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/send-pack.c b/send-pack.c
index 12e229e44..243633da1 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -133,10 +133,10 @@ static int pack_objects(int fd, struct ref *refs, struct
sha1_array *extra, stru
static int receive_unpack_status(int in)
{
const char *line = packet_read_line(in, NULL);
- if (!starts_with(line, "unpack "))
+ if (!skip_prefix(line, "unpack ", &line))
return error("did not receive remote status");
- if (strcmp(line, "unpack ok"))
- return error("unpack failed: %s", line + 7);
+ if (strcmp(line, "ok"))
+ return error("unpack failed: %s", line);
return 0;
}
--
2.12.0.429.gde83c8049
Re: fatal error when diffing changed symlinks
Johannes Schindelin writes:
>> > When viewing a working tree file, oid.hash could be 0{40} and
>> > read_sha1_file() is not the right function to use to obtain the
>> > contents.
>> >
>> > Both of these two need to pay attention to 0{40}, I think, as the
>> > user may be running "difftool -R --dir-diff" in which case the
>> > working tree would appear in the left hand side instead.
>>
>> As a side note, I think even outside of 0{40}, this should be checking
>> the return value of read_sha1_file(). A corrupted repo should die(), not
>> segfault.
>
> I agree. I am on it.
Friendly ping, if only to make sure that we can keep a piece of this
thread in the more "recent" pile.
If you have other topics you need to perfect, I think it is OK to
postpone the fix on this topic a bit longer, but I'd hate to ship
two releases with a known breakage without an attempt to fix it, so
if you are otherwise occupied, I may encourage others (including me)
to take a look at this. The new "difftool" also has a reported
regression somebody else expressed willingness to work on, which is
sort of blocked by everybody else not knowing the timeline on this
one. cf. <20170303212836.GB13790@arch-attack.localdomain>
A patch series would be very welcome, but "Please go ahead if
somebody else has time, and I'll help reviewing" would be also
good.
Thanks.
Re: [RESEND PATCH] git-gui--askpass: generalize the window title
https://public-inbox.org/git/xmqq60jz2xry@gitster.mtv.corp.google.com/ Although the following are included in git.git repository, they have their own authoritative repository and maintainers: - git-gui/ comes from git-gui project, maintained by Pat Thoyts: git://repo.or.cz/git-gui.git I cc'd Pat. Thanks, Stefan
[RESEND PATCH] git-gui--askpass: generalize the window title
git-gui--askpass is not only used for SSH authentication, but also for
HTTPS. In that context it is confusing to have a window title of
"OpenSSH". So generalize the title so that it also says which parent
process, i.e. Git, requires authentication.
Signed-off-by: Sebastian Schuberth
---
git-gui/git-gui--askpass | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-gui/git-gui--askpass b/git-gui/git-gui--askpass
index 4277f30..1e5c3256 100755
--- a/git-gui/git-gui--askpass
+++ b/git-gui/git-gui--askpass
@@ -60,7 +60,7 @@ proc finish {} {
set ::rc 0
}
-wm title . "OpenSSH"
+wm title . "Git Authentication"
tk::PlaceWindow .
vwait rc
exit $rc
--
https://github.com/git/git/pull/195
Re: [PATCH] t*: avoid using pipes
On Tue, Mar 7, 2017 at 8:10 AM, Prathamesh Chavan wrote: > Hi, > I'm Prathamesh Chavan. As a part of my micropraoject I have been working on > "Avoid pipes for git related commands in test suites". Thanks for working on that microproject! > I tried sending the > patch, but it got blocked since the mail contained more than 100 000 > characters. Yeah, even the github UI seems to have trouble with that commit. (A bit slow, not showing the full content, but rather I needed to click on "load diff" for tests files 7000+) This is a lot of change (in terms of lines) for a micro project. :) I'd have two competing advices: * keep it micro first, e.g. just convert one file, send to list, wait for reviewers feedback and incorporate that (optional step after having done the full development cycle: convert all the other files; each as its own patch) * split up this one patch into multiple patches, e.g. one file per patch, then send a patch series. The outcome will be the same, but in the first you get feedback quicker, such that hopefully you only need to touch the rest of files after the first file just once. > Hence I'll like to attach the link to my branch 'micro-proj', where I did the > required changes. > > https://github.com/pratham-pc/git/tree/micro-proj While I did look at that, not everyone here in the git community does so. (Also for getting your change in, Junio seems to strongly prefer patches on list instead of e.g. fetching and cherry-picking from your github) When looking at the content, the conversion seems a bit mechanical (which is fine for a micro project), such as: ... - test "$(git show --pretty=format:%s | head -n 1)" = "one" + test "$(git show --pretty=format:%s >out && head -n 1 > Thanks.
[PATCH] repack: Add options to preserve and prune old pack files
The new --preserve-oldpacks option moves old pack files into the
preserved subdirectory instead of deleting them after repacking.
The new --prune-preserved option prunes old pack files from the
preserved subdirectory after repacking, but before potentially
moving the latest old packfiles to this subdirectory.
These options are designed to prevent stale file handle exceptions
during git operations which can happen on users of NFS repos when
repacking is done on them. The strategy is to preserve old pack files
around until the next repack with the hopes that they will become
unreferenced by then and not cause any exceptions to running processes
when they are finally deleted (pruned).
Signed-off-by: James Melvin
---
Documentation/git-repack.txt | 9 +
builtin/repack.c | 38 --
2 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 26afe6ed5..0b19b761f 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -143,6 +143,15 @@ other objects in that pack they already have locally.
being removed. In addition, any unreachable loose objects will
be packed (and their loose counterparts removed).
+--preserve-oldpacks::
+Move old pack files into the preserved subdirectory instead
+of deleting them after repacking.
+
+--prune-preserved::
+Prune old pack files from the preserved subdirectory after
+repacking, but before potentially moving the latest old
+packfiles to this subdirectory
+
Configuration
-
diff --git a/builtin/repack.c b/builtin/repack.c
index 677bc7c81..f1a0c97f3 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -10,8 +10,10 @@
static int delta_base_offset = 1;
static int pack_kept_objects = -1;
+static int preserve_oldpacks = 0;
+static int prune_preserved = 0;
static int write_bitmaps;
-static char *packdir, *packtmp;
+static char *packdir, *packtmp, *preservedir;
static const char *const git_repack_usage[] = {
N_("git repack []"),
@@ -108,6 +110,27 @@ static void get_non_kept_pack_filenames(struct string_list
*fname_list)
closedir(dir);
}
+static void preserve_pack(const char *file_path, const char *file_name, const
char *file_ext)
+{
+ char *fname_old;
+
+ if (mkdir(preservedir, 0700) && errno != EEXIST)
+ error(_("failed to create preserve directory"));
+
+ fname_old = mkpathdup("%s/%s.old-%s", preservedir, file_name,
++file_ext);
+ rename(file_path, fname_old);
+
+ free(fname_old);
+}
+
+static void remove_preserved_dir(void) {
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_addstr(&buf, preservedir);
+ remove_dir_recursively(&buf, 0);
+ strbuf_release(&buf);
+}
+
static void remove_redundant_pack(const char *dir_name, const char *base_name)
{
const char *exts[] = {".pack", ".idx", ".keep", ".bitmap"};
@@ -121,7 +144,10 @@ static void remove_redundant_pack(const char *dir_name,
const char *base_name)
for (i = 0; i < ARRAY_SIZE(exts); i++) {
strbuf_setlen(&buf, plen);
strbuf_addstr(&buf, exts[i]);
- unlink(buf.buf);
+ if (preserve_oldpacks)
+ preserve_pack(buf.buf, base_name, exts[i]);
+ else
+ unlink(buf.buf);
}
strbuf_release(&buf);
}
@@ -194,6 +220,10 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
N_("maximum size of each packfile")),
OPT_BOOL(0, "pack-kept-objects", &pack_kept_objects,
N_("repack objects in packs marked with
.keep")),
+ OPT_BOOL(0, "preserve-oldpacks", &preserve_oldpacks,
+ N_("move old pack files into the preserved
subdirectory")),
+ OPT_BOOL(0, "prune-preserved", &prune_preserved,
+ N_("prune old pack files from the preserved
subdirectory after repacking")),
OPT_END()
};
@@ -217,6 +247,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
packdir = mkpathdup("%s/pack", get_object_directory());
packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
+ preservedir = mkpathdup("%s/preserved", packdir);
sigchain_push_common(remove_pack_on_signal);
@@ -404,6 +435,9 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
/* End of pack replacement. */
+ if (prune_preserved)
+ remove_preserved_dir();
+
if (delete_redundant) {
int opts = 0;
string_list_sort(&names);
--
2.12.0.190.gab6997d48.dirty
[PATCH] t*: avoid using pipes
Hi, I'm Prathamesh Chavan. As a part of my micropraoject I have been working on "Avoid pipes for git related commands in test suites". I tried sending the patch, but it got blocked since the mail contained more than 100 000 characters. Hence I'll like to attach the link to my branch 'micro-proj', where I did the required changes. https://github.com/pratham-pc/git/tree/micro-proj Thanks.
Re: Reg : GSoC 2017 Microproject
On Tue, Mar 7, 2017 at 3:52 PM, Vedant Bassi wrote: > Hi, > > I would like to participate in GSoC 2017 and I have chosen the Use > unsigned integral type for collection of bits , idea from the Micro > projects list. > > I request the help of the community for clarifying a few questions that I > have. > > 1. Is this Microproject already taken ? > > 2. If it is free , I would like to point out one place where a signed > int is used . > > In bisect.h , the structure struct rev_list_info uses flags of > type signed int but , the value of MSB is not checked as a test case > for any error checking. Hence it can be of type unsigned int. > It is only used in rev-list.c for checking cases (BISECT_SHOW_ALL and > REV_LIST_QUIET ). > > Is this a valid case. > > Thanks. You can search your microproject on public inbox of git to check if it has already been taken or not.
Re: [PATCH v3 0/9] Fix the early config
Hi Junio, On Fri, 3 Mar 2017, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Notable notes: > > > > - In contrast to earlier versions, I no longer special-case init and > > clone. Peff pointed out that this adds technical debt, and that we can > > actually argue (for consistency's sake) that early config reads the > > current repository config (if any) even for init and clone. > > > > - The read_early_config() function does not cache Git directory > > discovery nor read values. If needed, this can be implemented later, > > in a separate patch series. > > > > - The alias handling in git.c could possibly benefit from this work, but > > again, this is a separate topic from the current patch series. > > As Peff said in his review, I too find the result of this series a > more pleasant read than than original. As do I. > 2/9 and corresponding 4/9 triggers "ERROR: trailing statements > should be on next line" from ../linux/scripts/checkpatch.pl because > of a line inherited from the original; I'll queue them with an > obvious style fix to work it around. Thank you. I'll try to pick it up for v3 (which is needed, as I found another issue that needs to be fixed). Ciao, Johannes
[PATCH v4 07/10] read_early_config(): avoid .git/config hack when unneeded
So far, we only look whether the startup_info claims to have seen a
git_dir.
However, do_git_config_sequence() (and consequently the
git_config_with_options() call used by read_early_config() asks the
have_git_dir() function whether we have a .git/ directory, which in turn
also looks at git_dir and at the environment variable GIT_DIR. And when
this is the case, the repository config is handled already, so we do not
have to do that again explicitly.
Let's just use the same function, have_git_dir(), to determine whether we
have to handle .git/config explicitly.
Signed-off-by: Johannes Schindelin
---
config.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 9cfbeafd04c..068fa4dcfa6 100644
--- a/config.c
+++ b/config.c
@@ -1427,14 +1427,15 @@ void read_early_config(config_fn_t cb, void *data)
* core.repositoryformatversion), we'll read whatever is in .git/config
* blindly. Similarly, if we _are_ in a repository, but not at the
* root, we'll fail to find .git/config (because it's really
-* ../.git/config, etc). See t7006 for a complete set of failures.
+* ../.git/config, etc), unless setup_git_directory() was already
called.
+* See t7006 for a complete set of failures.
*
* However, we have historically provided this hack because it does
* work some of the time (namely when you are at the top-level of a
* valid repository), and would rarely make things worse (i.e., you do
* not generally have a .git/config file sitting around).
*/
- if (!startup_info->have_repository) {
+ if (!have_git_dir()) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
--
2.12.0.windows.1.7.g94dafc3b124
[PATCH v4 09/10] Test read_early_config()
So far, we had no explicit tests of that function.
Signed-off-by: Johannes Schindelin
---
t/helper/test-config.c | 15 +++
t/t1309-early-config.sh | 50 +
2 files changed, 65 insertions(+)
create mode 100755 t/t1309-early-config.sh
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 83a4f2ab869..8e3ed6a76cb 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -66,6 +66,16 @@ static int iterate_cb(const char *var, const char *value,
void *data)
return 0;
}
+static int early_config_cb(const char *var, const char *value, void *vdata)
+{
+ const char *key = vdata;
+
+ if (!strcmp(key, var))
+ printf("%s\n", value);
+
+ return 0;
+}
+
int cmd_main(int argc, const char **argv)
{
int i, val;
@@ -73,6 +83,11 @@ int cmd_main(int argc, const char **argv)
const struct string_list *strptr;
struct config_set cs;
+ if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
+ read_early_config(early_config_cb, (void *)argv[2]);
+ return 0;
+ }
+
setup_git_directory();
git_configset_init(&cs);
diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
new file mode 100755
index 000..0c55dee514c
--- /dev/null
+++ b/t/t1309-early-config.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='Test read_early_config()'
+
+. ./test-lib.sh
+
+test_expect_success 'read early config' '
+ test_config early.config correct &&
+ test-config read_early_config early.config >output &&
+ test correct = "$(cat output)"
+'
+
+test_expect_success 'in a sub-directory' '
+ test_config early.config sub &&
+ mkdir -p sub &&
+ (
+ cd sub &&
+ test-config read_early_config early.config
+ ) >output &&
+ test sub = "$(cat output)"
+'
+
+test_expect_success 'ceiling' '
+ test_config early.config ceiling &&
+ mkdir -p sub &&
+ (
+ GIT_CEILING_DIRECTORIES="$PWD" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd sub &&
+ test-config read_early_config early.config
+ ) >output &&
+ test -z "$(cat output)"
+'
+
+test_expect_success 'ceiling #2' '
+ mkdir -p xdg/git &&
+ git config -f xdg/git/config early.config xdg &&
+ test_config early.config ceiling &&
+ mkdir -p sub &&
+ (
+ XDG_CONFIG_HOME="$PWD"/xdg &&
+ GIT_CEILING_DIRECTORIES="$PWD" &&
+ export GIT_CEILING_DIRECTORIES XDG_CONFIG_HOME &&
+ cd sub &&
+ test-config read_early_config early.config
+ ) >output &&
+ test xdg = "$(cat output)"
+'
+
+test_done
--
2.12.0.windows.1.7.g94dafc3b124
Re: [PATCH 0/6] deadlock regression in v2.11.0 with failed mkdtemp
On Tue, Mar 07, 2017 at 08:34:37AM -0500, Jeff King wrote:
> Yuck. In the original, the error was generated by the child index-pack,
> and we relayed it over the sideband. But we don't even get as far as
> running index-pack in the newer version; we fail trying to make the
> tmpdir. The error ends up in the "unpack" protocol field, but the client
> side does a bad job of showing that. With the rest of the patches, it
> looks like:
>
> $ git push ~/tmp/foo.git HEAD
> Counting objects: 210973, done.
> Delta compression using up to 8 threads.
> Compressing objects: 100% (52799/52799), done.
> error: remote unpack failed: unable to create temporary object directory
> error: failed to push some refs to '/home/peff/tmp/foo.git'
There are two other options here I should mention.
One is that when pack-objects dies, we suppress the ref-status table
entirely. Which is fair, because it doesn't have anything interesting to
say. But for a case like this where the other side stops reading our
pack early but still produces status reports, we could actually read
them all and have a status table like:
$ git push ~/tmp/foo.git HEAD
Counting objects: 209843, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (52186/52186), done.
error: remote unpack failed: unable to create temporary object directory
To /home/peff/tmp/foo.git
! [remote rejected] HEAD -> jk/push-index-pack-deadlock (unpacker error)
error: failed to push some refs to '/home/peff/tmp/foo.git'
We'd have to take some care to make sure we handle the case when the
remote _doesn't_ manage to give us the status (e.g., when there's a
complete hangup). I don't know if it's worth the effort. There's no new
information there.
The second thing is that I think the design of the "unpack "
report is a bit dated. These days everybody supports the sideband
protocol, so it would probably make more sense to just issue the
"" report via the sideband (at which point it gets a nice
"remote: " prefix).
We could do something like this:
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f2c6953a3..6204d3d00 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1670,6 +1670,8 @@ static const char *unpack(int err_fd, struct shallow_info
*si)
if (!tmp_objdir) {
if (err_fd > 0)
close(err_fd);
+ rp_error("unable to create temporary object directory: %s",
+strerror(errno));
return "unable to create temporary object directory";
}
child.env = tmp_objdir_env(tmp_objdir);
and drop the "try to show the unpack failure" parts of my series, and
you'd end up with:
$ git push ~/tmp/foo.git HEAD
Counting objects: 209843, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (52186/52186), done.
remote: error: unable to create temporary object directory: Permission denied
error: failed to push some refs to '/home/peff/tmp/foo.git'
but in cases where pack-objects _doesn't_ fail, existing git versions
do show the "unpack" error. So you'd see it twice.
I don't know if it's worth trying to hack around.
-Peff
[PATCH v4 10/10] setup_git_directory_gently_1(): avoid die()ing
This function now has a new caller in addition to setup_git_directory():
the newly introduced discover_git_directory(). That function wants to
discover the current .git/ directory, and in case of a corrupted one
simply pretend that there is none to be found.
Example: if a stale .git file exists in the parent directory, and the
user calls `git -p init`, we want Git to simply *not* read any
repository config for the pager (instead of aborting with a message that
the .git file is corrupt).
Let's actually pretend that there was no GIT_DIR to be found in that case
when being called from discover_git_directory(), but keep the previous
behavior (i.e. to die()) for the setup_git_directory() case.
Signed-off-by: Johannes Schindelin
---
setup.c | 26 ++
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/setup.c b/setup.c
index 486acda2054..9118b48590a 100644
--- a/setup.c
+++ b/setup.c
@@ -825,7 +825,8 @@ enum discovery_result {
GIT_DIR_BARE,
/* these are errors */
GIT_DIR_HIT_CEILING = -1,
- GIT_DIR_HIT_MOUNT_POINT = -2
+ GIT_DIR_HIT_MOUNT_POINT = -2,
+ GIT_DIR_INVALID_GITFILE = -3
};
/*
@@ -842,7 +843,8 @@ enum discovery_result {
* `dir` (i.e. *not* necessarily the cwd).
*/
static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
- struct strbuf *gitdir)
+ struct strbuf *gitdir,
+ int die_on_error)
{
const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
@@ -890,14 +892,22 @@ static enum discovery_result
setup_git_directory_gently_1(struct strbuf *dir,
if (one_filesystem)
current_device = get_device_or_die(dir->buf, NULL, 0);
for (;;) {
- int offset = dir->len;
+ int offset = dir->len, error_code = 0;
if (offset > min_offset)
strbuf_addch(dir, '/');
strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
- gitdirenv = read_gitfile(dir->buf);
- if (!gitdirenv && is_git_directory(dir->buf))
- gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
+ gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
+ NULL : &error_code);
+ if (!gitdirenv) {
+ if (die_on_error ||
+ error_code == READ_GITFILE_ERR_NOT_A_FILE) {
+ if (is_git_directory(dir->buf))
+ gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
+ } else if (error_code &&
+ error_code != READ_GITFILE_ERR_STAT_FAILED)
+ return GIT_DIR_INVALID_GITFILE;
+ }
strbuf_setlen(dir, offset);
if (gitdirenv) {
strbuf_addstr(gitdir, gitdirenv);
@@ -934,7 +944,7 @@ const char *discover_git_directory(struct strbuf *gitdir)
return NULL;
cwd_len = dir.len;
- if (setup_git_directory_gently_1(&dir, gitdir) <= 0) {
+ if (setup_git_directory_gently_1(&dir, gitdir, 0) <= 0) {
strbuf_release(&dir);
return NULL;
}
@@ -993,7 +1003,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
die_errno(_("Unable to read current working directory"));
strbuf_addbuf(&dir, &cwd);
- switch (setup_git_directory_gently_1(&dir, &gitdir)) {
+ switch (setup_git_directory_gently_1(&dir, &gitdir, 1)) {
case GIT_DIR_NONE:
prefix = NULL;
break;
--
2.12.0.windows.1.7.g94dafc3b124
[PATCH v4 08/10] read_early_config(): really discover .git/
Earlier, we punted and simply assumed that we are in the top-level
directory of the project, and that there is no .git file but a .git/
directory so that we can read directly from .git/config.
However, that is not necessarily true. We may be in a subdirectory. Or
.git may be a gitfile. Or the environment variable GIT_DIR may be set.
To remedy this situation, we just refactored the way
setup_git_directory() discovers the .git/ directory, to make it
reusable, and more importantly, to leave all global variables and the
current working directory alone.
Let's discover the .git/ directory correctly in read_early_config() by
using that new function.
This fixes 4 known breakages in t7006.
Signed-off-by: Johannes Schindelin
---
config.c | 31 ---
t/t7006-pager.sh | 8
2 files changed, 16 insertions(+), 23 deletions(-)
diff --git a/config.c b/config.c
index 068fa4dcfa6..a88df53fdbc 100644
--- a/config.c
+++ b/config.c
@@ -1414,34 +1414,27 @@ static void configset_iter(struct config_set *cs,
config_fn_t fn, void *data)
void read_early_config(config_fn_t cb, void *data)
{
+ struct strbuf buf = STRBUF_INIT;
+
git_config_with_options(cb, data, NULL, 1);
/*
-* Note that this is a really dirty hack that does the wrong thing in
-* many cases. The crux of the problem is that we cannot run
-* setup_git_directory() early on in git's setup, so we have no idea if
-* we are in a repository or not, and therefore are not sure whether
-* and how to read repository-local config.
-*
-* So if we _aren't_ in a repository (or we are but we would reject its
-* core.repositoryformatversion), we'll read whatever is in .git/config
-* blindly. Similarly, if we _are_ in a repository, but not at the
-* root, we'll fail to find .git/config (because it's really
-* ../.git/config, etc), unless setup_git_directory() was already
called.
-* See t7006 for a complete set of failures.
-*
-* However, we have historically provided this hack because it does
-* work some of the time (namely when you are at the top-level of a
-* valid repository), and would rarely make things worse (i.e., you do
-* not generally have a .git/config file sitting around).
+* When setup_git_directory() was not yet asked to discover the
+* GIT_DIR, we ask discover_git_directory() to figure out whether there
+* is any repository config we should use (but unlike
+* setup_git_directory_gently(), no global state is changed, most
+* notably, the current working directory is still the same after the
+* call).
*/
- if (!have_git_dir()) {
+ if (!have_git_dir() && discover_git_directory(&buf)) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
- repo_config.file = ".git/config";
+ strbuf_addstr(&buf, "/config");
+ repo_config.file = buf.buf;
git_config_with_options(cb, data, &repo_config, 1);
}
+ strbuf_release(&buf);
}
static void git_config_check_init(void);
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 304ae06c600..4f3794d415e 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -360,19 +360,19 @@ test_pager_choices 'git aliasedlog'
test_default_pagerexpect_success 'git -p aliasedlog'
test_PAGER_overrides expect_success 'git -p aliasedlog'
test_core_pager_overrides expect_success 'git -p aliasedlog'
-test_core_pager_subdirexpect_failure 'git -p aliasedlog'
+test_core_pager_subdirexpect_success 'git -p aliasedlog'
test_GIT_PAGER_overrides expect_success 'git -p aliasedlog'
test_default_pagerexpect_success 'git -p true'
test_PAGER_overrides expect_success 'git -p true'
test_core_pager_overrides expect_success 'git -p true'
-test_core_pager_subdirexpect_failure 'git -p true'
+test_core_pager_subdirexpect_success 'git -p true'
test_GIT_PAGER_overrides expect_success 'git -p true'
test_default_pagerexpect_success test_must_fail 'git -p request-pull'
test_PAGER_overrides expect_success test_must_fail 'git -p request-pull'
test_core_pager_overrides expect_success test_must_fail 'git -p request-pull'
-test_core_pager_subdirexpect_failure test_must_fail 'git -p request-pull'
+test_core_pager_subdirexpect_success test_must_fail 'git -p request-pull'
test_GIT_PAGER_overrides expect_success test_must_fail 'git -p request-pull'
test_default_pagerexpect_success test_must_fail 'git -p'
@@ -380,7 +380,7 @@ test_PAGER_overrides expect_success test_must_fail
'git -p'
test_local_config_ignored expect_failure test_must_fail 'git -p'
test_GIT_PAGER_overrides expect_success test_must_fail 'git -p'
-test_expect_failure TTY 'core.pager in repo con
[PATCH v4 05/10] Introduce the discover_git_directory() function
We modified the setup_git_directory_gently_1() function earlier to make
it possible to discover the GIT_DIR without changing global state.
However, it is still a bit cumbersome to use if you only need to figure
out the (possibly absolute) path of the .git/ directory. Let's just
provide a convenient wrapper function with an easier signature that
*just* discovers the .git/ directory.
We will use it in a subsequent patch to fix the early config.
Signed-off-by: Johannes Schindelin
---
cache.h | 7 +++
setup.c | 43 +++
2 files changed, 50 insertions(+)
diff --git a/cache.h b/cache.h
index 80b6372cf76..5218726cf88 100644
--- a/cache.h
+++ b/cache.h
@@ -518,6 +518,13 @@ extern void set_git_work_tree(const char *tree);
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
extern void setup_work_tree(void);
+/*
+ * Find GIT_DIR of the repository that contains the current working directory,
+ * without changing the working directory or other global state. The result is
+ * appended to gitdir. The return value is either NULL if no repository was
+ * found, or pointing to the path inside gitdir's buffer.
+ */
+extern const char *discover_git_directory(struct strbuf *gitdir);
extern const char *setup_git_directory_gently(int *);
extern const char *setup_git_directory(void);
extern char *prefix_path(const char *prefix, int len, const char *path);
diff --git a/setup.c b/setup.c
index d7af343d14e..486acda2054 100644
--- a/setup.c
+++ b/setup.c
@@ -924,6 +924,49 @@ static enum discovery_result
setup_git_directory_gently_1(struct strbuf *dir,
}
}
+const char *discover_git_directory(struct strbuf *gitdir)
+{
+ struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
+ size_t gitdir_offset = gitdir->len, cwd_len;
+ struct repository_format candidate;
+
+ if (strbuf_getcwd(&dir))
+ return NULL;
+
+ cwd_len = dir.len;
+ if (setup_git_directory_gently_1(&dir, gitdir) <= 0) {
+ strbuf_release(&dir);
+ return NULL;
+ }
+
+ /*
+* The returned gitdir is relative to dir, and if dir does not reflect
+* the current working directory, we simply make the gitdir absolute.
+*/
+ if (dir.len < cwd_len && !is_absolute_path(gitdir->buf +
gitdir_offset)) {
+ /* Avoid a trailing "/." */
+ if (!strcmp(".", gitdir->buf + gitdir_offset))
+ strbuf_setlen(gitdir, gitdir_offset);
+ else
+ strbuf_addch(&dir, '/');
+ strbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);
+ }
+
+ strbuf_reset(&dir);
+ strbuf_addf(&dir, "%s/config", gitdir->buf + gitdir_offset);
+ read_repository_format(&candidate, dir.buf);
+ strbuf_release(&dir);
+
+ if (verify_repository_format(&candidate, &err) < 0) {
+ warning("ignoring git dir '%s': %s",
+ gitdir->buf + gitdir_offset, err.buf);
+ strbuf_release(&err);
+ return NULL;
+ }
+
+ return gitdir->buf + gitdir_offset;
+}
+
const char *setup_git_directory_gently(int *nongit_ok)
{
struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, gitdir =
STRBUF_INIT;
--
2.12.0.windows.1.7.g94dafc3b124
[PATCH v4 00/10] Fix the early config
These patches are an attempt to make Git's startup sequence a bit less surprising. The idea here is to discover the .git/ directory gently (i.e. without changing the current working directory, nor any global variables), and to use it to read the .git/config file early, before we actually called setup_git_directory() (if we ever do that). This also allows us to fix the early config e.g. to determine the pager or to resolve aliases in a non-surprising manner. My own use case: in the GVFS Git fork, we need to execute pre-command and post-command hooks before and after *every* Git command. A previous version of the pre-command/post-command hook support was broken, as it used run_hook() which implicitly called setup_git_directory() too early. The discover_git_directory() function (and due to core.hooksPath also the read_early_config() function) helped me fix this. Notable notes: - Even if it can cause surprising problems, `init` and `clone` are not special-cased. Rationale: it would introduce technical debt and violate the Principle Of Least Astonishment. - The read_early_config() function does not cache Git directory discovery nor read values. This is left for another patch series, if it ever becomes necessary. - The alias handling in git.c could possibly benefit from this work, but again, this is a separate topic from the current patch series. Changes since v3: - instead of just fixing the `== '/'` comparison, 2/9 now *also* changes the style of the original to something Linux' checkpatch.pl prefers. - fixed the comment added in 8/9 which was stale from an earlier iteration of this patch series. - adjusted the commit above discover_git_directory()'s declaration, to make it more understandable. - touched up the commit message of 5/9 to make the flow more natural. - moved a fault fixup: by mistake, the repository_format check was added to 8/9 when it really needed to go to 5/9, where the discover_git_directory() function was introduced (I noticed this while rebasing). - fixed discover_git_directory() when setup_git_directory_gently_1() returned GIT_DIR_NONE (the previous iteration would *not* return NULL in that case). Johannes Schindelin (10): t7006: replace dubious test setup_git_directory(): use is_dir_sep() helper Prepare setup_discovered_git_directory() the root directory setup_git_directory_1(): avoid changing global state Introduce the discover_git_directory() function Make read_early_config() reusable read_early_config(): avoid .git/config hack when unneeded read_early_config(): really discover .git/ Test read_early_config() setup_git_directory_gently_1(): avoid die()ing cache.h | 8 ++ config.c| 25 + pager.c | 31 -- setup.c | 246 +--- t/helper/test-config.c | 15 +++ t/t1309-early-config.sh | 50 ++ t/t7006-pager.sh| 18 +++- 7 files changed, 282 insertions(+), 111 deletions(-) create mode 100755 t/t1309-early-config.sh base-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858 Published-As: https://github.com/dscho/git/releases/tag/early-config-v4 Fetch-It-Via: git fetch https://github.com/dscho/git early-config-v4 Interdiff vs v3: diff --git a/cache.h b/cache.h index 8a4580f921d..e7b57457e73 100644 --- a/cache.h +++ b/cache.h @@ -518,7 +518,12 @@ extern void set_git_work_tree(const char *tree); #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES" extern void setup_work_tree(void); -/* Find GIT_DIR without changing the working directory or other global state */ +/* + * Find GIT_DIR of the repository that contains the current working directory, + * without changing the working directory or other global state. The result is + * appended to gitdir. The return value is either NULL if no repository was + * found, or pointing to the path inside gitdir's buffer. + */ extern const char *discover_git_directory(struct strbuf *gitdir); extern const char *setup_git_directory_gently(int *); extern const char *setup_git_directory(void); diff --git a/config.c b/config.c index 749623a9649..a88df53fdbc 100644 --- a/config.c +++ b/config.c @@ -1419,14 +1419,12 @@ void read_early_config(config_fn_t cb, void *data) git_config_with_options(cb, data, NULL, 1); /* - * When we are not about to create a repository ourselves (init or - * clone) and when no .git/ directory was set up yet (in which case - * git_config_with_options() would already have picked up the - * repository config), we ask discover_git_directory() to figure out - * whether there is any repository config we should use (but unlike + * When setup_git_directory() was not yet asked to discover the + * GIT_DIR, we ask discover_git_directory() to figure out whether there + * is any repository config we should use (but unlike * setup
[PATCH v4 06/10] Make read_early_config() reusable
The pager configuration needs to be read early, possibly before
discovering any .git/ directory.
Let's not hide this function in pager.c, but make it available to other
callers.
Signed-off-by: Johannes Schindelin
---
cache.h | 1 +
config.c | 31 +++
pager.c | 31 ---
3 files changed, 32 insertions(+), 31 deletions(-)
diff --git a/cache.h b/cache.h
index 5218726cf88..e7b57457e73 100644
--- a/cache.h
+++ b/cache.h
@@ -1804,6 +1804,7 @@ extern int git_config_from_blob_sha1(config_fn_t fn,
const char *name,
const unsigned char *sha1, void *data);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
+extern void read_early_config(config_fn_t cb, void *data);
extern void git_config(config_fn_t fn, void *);
extern int git_config_with_options(config_fn_t fn, void *,
struct git_config_source *config_source,
diff --git a/config.c b/config.c
index c6b874a7bf7..9cfbeafd04c 100644
--- a/config.c
+++ b/config.c
@@ -1412,6 +1412,37 @@ static void configset_iter(struct config_set *cs,
config_fn_t fn, void *data)
}
}
+void read_early_config(config_fn_t cb, void *data)
+{
+ git_config_with_options(cb, data, NULL, 1);
+
+ /*
+* Note that this is a really dirty hack that does the wrong thing in
+* many cases. The crux of the problem is that we cannot run
+* setup_git_directory() early on in git's setup, so we have no idea if
+* we are in a repository or not, and therefore are not sure whether
+* and how to read repository-local config.
+*
+* So if we _aren't_ in a repository (or we are but we would reject its
+* core.repositoryformatversion), we'll read whatever is in .git/config
+* blindly. Similarly, if we _are_ in a repository, but not at the
+* root, we'll fail to find .git/config (because it's really
+* ../.git/config, etc). See t7006 for a complete set of failures.
+*
+* However, we have historically provided this hack because it does
+* work some of the time (namely when you are at the top-level of a
+* valid repository), and would rarely make things worse (i.e., you do
+* not generally have a .git/config file sitting around).
+*/
+ if (!startup_info->have_repository) {
+ struct git_config_source repo_config;
+
+ memset(&repo_config, 0, sizeof(repo_config));
+ repo_config.file = ".git/config";
+ git_config_with_options(cb, data, &repo_config, 1);
+ }
+}
+
static void git_config_check_init(void);
void git_config(config_fn_t fn, void *data)
diff --git a/pager.c b/pager.c
index ae796433630..73ca8bc3b17 100644
--- a/pager.c
+++ b/pager.c
@@ -43,37 +43,6 @@ static int core_pager_config(const char *var, const char
*value, void *data)
return 0;
}
-static void read_early_config(config_fn_t cb, void *data)
-{
- git_config_with_options(cb, data, NULL, 1);
-
- /*
-* Note that this is a really dirty hack that does the wrong thing in
-* many cases. The crux of the problem is that we cannot run
-* setup_git_directory() early on in git's setup, so we have no idea if
-* we are in a repository or not, and therefore are not sure whether
-* and how to read repository-local config.
-*
-* So if we _aren't_ in a repository (or we are but we would reject its
-* core.repositoryformatversion), we'll read whatever is in .git/config
-* blindly. Similarly, if we _are_ in a repository, but not at the
-* root, we'll fail to find .git/config (because it's really
-* ../.git/config, etc). See t7006 for a complete set of failures.
-*
-* However, we have historically provided this hack because it does
-* work some of the time (namely when you are at the top-level of a
-* valid repository), and would rarely make things worse (i.e., you do
-* not generally have a .git/config file sitting around).
-*/
- if (!startup_info->have_repository) {
- struct git_config_source repo_config;
-
- memset(&repo_config, 0, sizeof(repo_config));
- repo_config.file = ".git/config";
- git_config_with_options(cb, data, &repo_config, 1);
- }
-}
-
const char *git_pager(int stdout_is_tty)
{
const char *pager;
--
2.12.0.windows.1.7.g94dafc3b124
[PATCH v4 04/10] setup_git_directory_1(): avoid changing global state
For historical reasons, Git searches for the .git/ directory (or the
.git file) by changing the working directory successively to the parent
directory of the current directory, until either anything was found or
until a ceiling or a mount point is hit.
Further global state may be changed in case a .git/ directory was found.
We do have a use case, though, where we would like to find the .git/
directory without having any global state touched, though: when we read
the early config e.g. for the pager or for alias expansion.
Let's just move all of code that changes any global state out of the
function `setup_git_directory_gently_1()` into
`setup_git_directory_gently()`.
In subsequent patches, we will use the _1() function in a new
`discover_git_directory()` function that we will then use for the early
config code.
Note: the new loop is a *little* tricky, as we have to handle the root
directory specially: we cannot simply strip away the last component
including the slash, as the root directory only has that slash. To remedy
that, we introduce the `min_offset` variable that holds the minimal length
of an absolute path, and using that to special-case the root directory,
including an early exit before trying to find the parent of the root
directory.
Signed-off-by: Johannes Schindelin
---
setup.c | 187 ++--
1 file changed, 112 insertions(+), 75 deletions(-)
diff --git a/setup.c b/setup.c
index 20a1f0f870e..d7af343d14e 100644
--- a/setup.c
+++ b/setup.c
@@ -818,50 +818,49 @@ static int canonicalize_ceiling_entry(struct
string_list_item *item,
}
}
+enum discovery_result {
+ GIT_DIR_NONE = 0,
+ GIT_DIR_EXPLICIT,
+ GIT_DIR_DISCOVERED,
+ GIT_DIR_BARE,
+ /* these are errors */
+ GIT_DIR_HIT_CEILING = -1,
+ GIT_DIR_HIT_MOUNT_POINT = -2
+};
+
/*
* We cannot decide in this function whether we are in the work tree or
* not, since the config can only be read _after_ this function was called.
+ *
+ * Also, we avoid changing any global state (such as the current working
+ * directory) to allow early callers.
+ *
+ * The directory where the search should start needs to be passed in via the
+ * `dir` parameter; upon return, the `dir` buffer will contain the path of
+ * the directory where the search ended, and `gitdir` will contain the path of
+ * the discovered .git/ directory, if any. This path may be relative against
+ * `dir` (i.e. *not* necessarily the cwd).
*/
-static const char *setup_git_directory_gently_1(int *nongit_ok)
+static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
+ struct strbuf *gitdir)
{
const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
- static struct strbuf cwd = STRBUF_INIT;
- const char *gitdirenv, *ret;
- char *gitfile;
- int offset, offset_parent, ceil_offset = -1;
+ const char *gitdirenv;
+ int ceil_offset = -1, min_offset = has_dos_drive_prefix(dir->buf) ? 3 :
1;
dev_t current_device = 0;
int one_filesystem = 1;
/*
-* We may have read an incomplete configuration before
-* setting-up the git directory. If so, clear the cache so
-* that the next queries to the configuration reload complete
-* configuration (including the per-repo config file that we
-* ignored previously).
-*/
- git_config_clear();
-
- /*
-* Let's assume that we are in a git repository.
-* If it turns out later that we are somewhere else, the value will be
-* updated accordingly.
-*/
- if (nongit_ok)
- *nongit_ok = 0;
-
- if (strbuf_getcwd(&cwd))
- die_errno(_("Unable to read current working directory"));
- offset = cwd.len;
-
- /*
* If GIT_DIR is set explicitly, we're not going
* to do any discovery, but we still do repository
* validation.
*/
gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
- if (gitdirenv)
- return setup_explicit_git_dir(gitdirenv, &cwd, nongit_ok);
+ if (gitdirenv) {
+ strbuf_addstr(gitdir, gitdirenv);
+ return GIT_DIR_EXPLICIT;
+ }
if (env_ceiling_dirs) {
int empty_entry_found = 0;
@@ -869,15 +868,15 @@ static const char *setup_git_directory_gently_1(int
*nongit_ok)
string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP,
-1);
filter_string_list(&ceiling_dirs, 0,
canonicalize_ceiling_entry,
&empty_entry_found);
- ceil_offset = longest_ancestor_length(cwd.buf, &ceiling_dirs);
+ ceil_offset = longest_ancestor_length(dir->buf, &ceiling_dirs);
string_list_clear(&ceiling_dirs, 0)
[PATCH v4 03/10] Prepare setup_discovered_git_directory() the root directory
Currently, the offset parameter (indicating what part of the cwd parameter corresponds to the current directory after discovering the .git/ directory) is set to 0 when we are running in the root directory. However, in the next patches we will avoid changing the current working directory while searching for the .git/ directory, meaning that the offset corresponding to the root directory will have to be 1 to reflect that this directory is characterized by the path "/" (and not ""). So let's make sure that setup_discovered_git_directory() only tries to append the trailing slash to non-root directories. Note: the setup_bare_git_directory() does not need a corresponding change, as it does not want to return a prefix. Signed-off-by: Johannes Schindelin --- setup.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/setup.c b/setup.c index 4a15b105676..20a1f0f870e 100644 --- a/setup.c +++ b/setup.c @@ -721,8 +721,10 @@ static const char *setup_discovered_git_dir(const char *gitdir, if (offset == cwd->len) return NULL; - /* Make "offset" point to past the '/', and add a '/' at the end */ - offset++; + /* Make "offset" point past the '/' (already the case for root dirs) */ + if (offset != offset_1st_component(cwd->buf)) + offset++; + /* Add a '/' at the end */ strbuf_addch(cwd, '/'); return cwd->buf + offset; } -- 2.12.0.windows.1.7.g94dafc3b124
[PATCH v4 01/10] t7006: replace dubious test
The idea of the test case "git -p - core.pager is not used from subdirectory" was to verify that the setup_git_directory() function had not been called just to obtain the core.pager setting. However, we are about to fix the early config machinery so that it *does* work, without messing up the global state. Once that is done, the core.pager setting *will* be used, even when running from a subdirectory, and that is a Good Thing. The intention of that test case, however, was to verify that the setup_git_directory() function has not run, because it changes global state such as the current working directory. To keep that spirit, but fix the incorrect assumption, this patch replaces that test case by a new one that verifies that the pager is run in the subdirectory, i.e. that the current working directory has not been changed at the time the pager is configured and launched, even if the `rev-parse` command requires a .git/ directory and *will* change the working directory. Signed-off-by: Johannes Schindelin --- t/t7006-pager.sh | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh index c8dc665f2fd..304ae06c600 100755 --- a/t/t7006-pager.sh +++ b/t/t7006-pager.sh @@ -378,9 +378,19 @@ test_GIT_PAGER_overrides expect_success test_must_fail 'git -p request-pull' test_default_pagerexpect_success test_must_fail 'git -p' test_PAGER_overrides expect_success test_must_fail 'git -p' test_local_config_ignored expect_failure test_must_fail 'git -p' -test_no_local_config_subdir expect_success test_must_fail 'git -p' test_GIT_PAGER_overrides expect_success test_must_fail 'git -p' +test_expect_failure TTY 'core.pager in repo config works and retains cwd' ' + sane_unset GIT_PAGER && + test_config core.pager "cat >cwd-retained" && + ( + cd sub && + rm -f cwd-retained && + test_terminal git -p rev-parse HEAD && + test_path_is_file cwd-retained + ) +' + test_doesnt_paginate expect_failure test_must_fail 'git -p nonsense' test_pager_choices 'git shortlog' -- 2.12.0.windows.1.7.g94dafc3b124
[PATCH v4 02/10] setup_git_directory(): use is_dir_sep() helper
It is okay in practice to test for forward slashes in the output of
getcwd(), because we go out of our way to convert backslashes to forward
slashes in getcwd()'s output on Windows.
Still, the correct way to test for a dir separator is by using the
helper function we introduced for that very purpose. It also serves as a
good documentation what the code tries to do (not "how").
Signed-off-by: Johannes Schindelin
---
setup.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/setup.c b/setup.c
index 967f289f1ef..4a15b105676 100644
--- a/setup.c
+++ b/setup.c
@@ -910,7 +910,9 @@ static const char *setup_git_directory_gently_1(int
*nongit_ok)
return setup_bare_git_dir(&cwd, offset, nongit_ok);
offset_parent = offset;
- while (--offset_parent > ceil_offset && cwd.buf[offset_parent]
!= '/');
+ while (--offset_parent > ceil_offset &&
+ !is_dir_sep(cwd.buf[offset_parent]))
+ ; /* continue */
if (offset_parent <= ceil_offset)
return setup_nongit(cwd.buf, nongit_ok);
if (one_filesystem) {
--
2.12.0.windows.1.7.g94dafc3b124
Re: [PATCH v3 0/9] Fix the early config
Hi Peff,
On Sat, 4 Mar 2017, Jeff King wrote:
> On Fri, Mar 03, 2017 at 06:31:55PM +0100, Johannes Schindelin wrote:
>
> > Interdiff vs v2:
> > [...]
> > + * When we are not about to create a repository ourselves (init or
> > + * clone) and when no .git/ directory was set up yet (in which case
> > + * git_config_with_options() would already have picked up the
> > + * repository config), we ask discover_git_directory() to figure out
> > + * whether there is any repository config we should use (but unlike
> > + * setup_git_directory_gently(), no global state is changed, most
> > + * notably, the current working directory is still the same after
> > + * the call).
> > */
> > - if (!startup_info->creating_repository && !have_git_dir() &&
> > - discover_git_directory(&buf)) {
> > + if (!have_git_dir() && discover_git_directory(&buf)) {
>
> I think this "when we are not about to..." part of the comment is no
> longer true, given the second part of the hunk.
Yep, that was a stale part of that patch. Thanks for noticing!
> > @@ -721,8 +721,10 @@ static const char *setup_discovered_git_dir(const
> > char *gitdir,
> > if (offset == cwd->len)
> > return NULL;
> >
> > - /* Make "offset" point to past the '/', and add a '/' at the end */
> > - offset++;
> > + /* Make "offset" point past the '/' (already the case for root dirs) */
> > + if (offset != offset_1st_component(cwd->buf))
> > + offset++;
>
> Nice. I was worried we would have to have a hacky "well, sometimes we
> don't add one here..." code, but using offset_1st_component says
> exactly what we mean.
Right. I also wanted to avoid that very, very much. My initial version
actually tried to detect whether cwd already has a trailing slash, but
then I figured that we can be much, much more precise here (and I am
really pleased how offset_1st_component() is *semantically* precise, i.e.
it describes very well what the code is supposed to do here).
> > +/* Find GIT_DIR without changing the working directory or other global
> > state */
> > extern const char *discover_git_directory(struct strbuf *gitdir);
>
> The parts that actually confused me were the parameters (mostly whether
> gitdir was a directory to start looking in, or an output parameter). So
> maybe:
>
> /*
>* Find GIT_DIR of the repository that contains the current working
>* directory, without changing the working directory or other global
>* state. The result is appended to gitdir. The return value is NULL
>* if no repository was found, or gitdir->buf otherwise.
>*/
I changed it a little bit more. In particular, I changed the
discover_git_directory() function to return the pointer to the path
itself: it provides additional value, and if that is not what the caller
wants, they can use git_dir->buf just as well.
> This looks good to me aside from those few comment nits.
Thanks.
It is not obvious from the interdiff, but I had an incorrect fixup to 8/9
that actually wanted to go to 5/9: the code in
discover_git_repository() tests the repository version should be part of
the initial version of this function, of course.
There is one more thing I included in v4: when I (re-)implemented that
pre-command/post-command hook I was hinting at earlier, the test suite
identified a problem where an invalid .git file would prevent even `git
init` from working (it was actually much more complicated than that, but
the gist is that `git -p init` would fail, no matter how much sense it
may make to you to paginate an `init` run, it should still not fail,
right?). So I added a patch on top to fix that.
And another change: the GIT_DIR_NONE value was handled incorrectly in
discover_git_directory().
I am slightly disappointed that the these additional problems were not
spotted in any review but my own. And I had not even included a Duck.
> I'm still not sure I understand how ceil_offset works in
> setup_git_directory_gently_1(), but I don't think your patch actually
> changed it. I can live with my confusion.
Yes, that code is very confusing. It also does not help that the naming is
inconsistent in that it abbreviates "ceiling" but not "offset". What makes
it even worse is that the function name `longest_ancestor_length()` is
highly misleading: in Git's context, "ancestor" of sth pretty much always
refers to a commit reachable from sth, but in this context it refers to
the path of a directory containing sth.
So basically, we set ceil_offset to the offset of the last directory
separator in our path that corresponds to the most precise match in
GIT_CEILING_DIRECTORIES.
Example: given GIT_CEILING_DIRECTORIES /foo:/:/bar and a path of /foo/bar,
ceil_offset would be 4, pointing to the slash at the end of /foo/ because
that is the most precise match in GIT_CEILING_DIRECTORIES ("/" would also
match, but is less precise).
Later, setup_git_directory_gently_1() ensures that it does not go beyond
ceil_offset when lookin
[PATCH 4/6] send-pack: improve unpack-status error messages
When the remote tells us that the "unpack" step failed, we
show an error message. However, unless you are familiar with
the internals of send-pack and receive-pack, it was not
clear that this represented an error on the remote side.
Let's re-word to make that more obvious.
Likewise, when we got an unexpected packet from the other
end, we complained with a vague message but did not actually
show the packet. Let's fix that.
And finally, neither message was marked for translation. The
message from the remote probably won't be translated, but
there's no reason we can't do better for the local half.
Signed-off-by: Jeff King
---
send-pack.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/send-pack.c b/send-pack.c
index 243633da1..83c23aef6 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -134,9 +134,9 @@ static int receive_unpack_status(int in)
{
const char *line = packet_read_line(in, NULL);
if (!skip_prefix(line, "unpack ", &line))
- return error("did not receive remote status");
+ return error(_("unable to parse remote unpack status: %s"),
line);
if (strcmp(line, "ok"))
- return error("unpack failed: %s", line);
+ return error(_("remote unpack failed: %s"), line);
return 0;
}
--
2.12.0.429.gde83c8049
[PATCH 5/6] send-pack: read "unpack" status even on pack-objects failure
If the local pack-objects of a push fails, we'll tell the
user about it. But one likely cause is that the remote
index-pack stopped reading for some reason (because it
didn't like our input, or encountered another error). In
that case we'd expect the remote to report more details to
us via the "unpack ..." status line. However, the current
code just hangs up completely, and the user never sees it.
Instead, let's call receive_unpack_status(), which will
complain on stderr with whatever reason the remote told us.
Note that if our pack-objects fails because the connection
was severed or the remote just crashed entirely, then our
packet_read_line() call may fail with "the remote end hung
up unexpectedly". That's OK. It's a more accurate
description than what we get now (which is just "some refs
failed to push").
This should be safe from any deadlocks. At the point we make
this call we'll have closed the writing end of the
connection to the server (either by handing it off to
a pack-objects which exited, explicitly in the stateless_rpc
case, or by doing a half-duplex shutdown for a socket). So
there should be no chance that the other side is waiting
for the rest of our pack-objects input.
Signed-off-by: Jeff King
---
send-pack.c | 8
1 file changed, 8 insertions(+)
diff --git a/send-pack.c b/send-pack.c
index 83c23aef6..e15232739 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -562,6 +562,14 @@ int send_pack(struct send_pack_args *args,
close(out);
if (git_connection_is_socket(conn))
shutdown(fd[0], SHUT_WR);
+
+ /*
+* Do not even bother with the return value; we know we
+* are failing, and just want the error() side effects.
+*/
+ if (status_report)
+ receive_unpack_status(in);
+
if (use_sideband) {
close(demux.out);
finish_async(&demux);
--
2.12.0.429.gde83c8049
Re: [Request for Documentation] Differentiate signed (commits/tags/pushes)
We use git to manage a config management repository for some servers. We have tens of signed commits a day; all get deployed. The logic on each host is roughly "is signed by sysadmin key and is more recent than currently-deployed version". Also, what is all this about "GPG"? The protocol is OpenPGP. A particular implementation is GnuPG / gpg. It is completely mad that this implementation detail is in the interface specs for git, such as --gpg-sign for git-commit(1). It is an indictment of a lack of appreciation of the relationship between interfaces and implementations, and the importance of proper treatment thereof. If Bob creates Bob's git compatible program, and he happens to use Bob's OpenPGP implementation, his compatible option for git-commit(1) still has to be called "--gpg-sign". Madness. Tom.
[PATCH 6/6] send-pack: report signal death of pack-objects
If our pack-objects sub-process dies of a signal, then it
likely didn't have a chance to write anything useful to
stderr. The user may be left scratching their head why the
push failed. Let's detect this situation and write something
to stderr.
Signed-off-by: Jeff King
---
We could drop the SIGPIPE special-case, but I think it's just noise
after the unpack-status fix in the previous commit.
send-pack.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/send-pack.c b/send-pack.c
index e15232739..d2d2a49a0 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -72,6 +72,7 @@ static int pack_objects(int fd, struct ref *refs, struct
sha1_array *extra, stru
struct child_process po = CHILD_PROCESS_INIT;
FILE *po_in;
int i;
+ int rc;
i = 4;
if (args->use_thin_pack)
@@ -125,8 +126,20 @@ static int pack_objects(int fd, struct ref *refs, struct
sha1_array *extra, stru
po.out = -1;
}
- if (finish_command(&po))
+ rc = finish_command(&po);
+ if (rc) {
+ /*
+* For a normal non-zero exit, we assume pack-objects wrote
+* something useful to stderr. For death by signal, though,
+* we should mention it to the user. The exception is SIGPIPE
+* (141), because that's a normal occurence if the remote end
+* hangs up (and we'll report that by trying to read the unpack
+* status).
+*/
+ if (rc > 128 && rc != 141)
+ error("pack-objects died of signal %d", rc - 128);
return -1;
+ }
return 0;
}
--
2.12.0.429.gde83c8049
regression: git push in non-shared repo stalls (v2.11.0+)
Hi, I observe a regression that seems to have been introduced between v2.10.0 and v2.11.0. When I try to push into a repository on the local filesystem that belongs to another user and has not explicitly been prepared for shared use, v2.11.0 shows some of the usual diagnostic output and then freezes instead of announcing why it failed to push. Horst Steps to reproduce (tested on Debian 8 "Jessie" amd64): - User A creates a bare repository: mkdir /tmp/gittest git init --bare /tmp/gittest - User B clones it, adds and commits a file: git clone /tmp/gittest cd gittest echo 42 > x git add x git commit -m test - User B tries to push to user A's bare repo: git push Expected result (git v2.10.0 and earlier): test@ios:~/gittest$ git push Counting objects: 3, done. Writing objects: 100% (3/3), 230 bytes | 0 bytes/s, done. Total 3 (delta 0), reused 0 (delta 0) remote: error: insufficient permission for adding an object to repository database objects remote: fatal: failed to write object error: unpack failed: unpack-objects abnormal exit To /tmp/gittest ! [remote rejected] master -> master (unpacker error) error: failed to push some refs to '/tmp/gittest' Actual result (git v2.11.0, v2.12.0, and 2.12.0.189.g3bc53220c): test@ios:~/gittest$ git push Counting objects: 3, done. Writing objects: 100% (3/3), 230 bytes | 0 bytes/s, done. Total 3 (delta 0), reused 0 (delta 0) [... git freezes here ...] -- PGP-Key 0xD40E0E7A
[PATCH 1/6] receive-pack: fix deadlock when we cannot create tmpdir
The err_fd descriptor passed to the unpack() function is
intended to be handed off to the child index-pack, and our
async muxer will read until it gets EOF. However, if we
encounter an error before handing off the descriptor, we
must manually close(err_fd). Otherwise we will be waiting
for our muxer to finish, while the muxer is waiting for EOF
on err_fd.
We fixed an identical deadlock already in 49ecfa13f
(receive-pack: close sideband fd on early pack errors,
2013-04-19). But since then, the function grew a new
early-return in 722ff7f87 (receive-pack: quarantine objects
until pre-receive accepts, 2016-10-03), when we fail to
create a temporary directory. This return needs the same
treatment.
Reported-by: Horst Schirmeier
Signed-off-by: Jeff King
---
builtin/receive-pack.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9ed8fbbfa..f2c6953a3 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1667,8 +1667,11 @@ static const char *unpack(int err_fd, struct
shallow_info *si)
}
tmp_objdir = tmp_objdir_create();
- if (!tmp_objdir)
+ if (!tmp_objdir) {
+ if (err_fd > 0)
+ close(err_fd);
return "unable to create temporary object directory";
+ }
child.env = tmp_objdir_env(tmp_objdir);
/*
--
2.12.0.429.gde83c8049
[PATCH 2/6] send-pack: extract parsing of "unpack" response
After sending the pack, we call receive_status() which gets
both the "unpack" line and the ref status. Let's break these
into two functions so we can call the first part
independently.
Signed-off-by: Jeff King
---
send-pack.c | 23 ++-
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/send-pack.c b/send-pack.c
index 6195b43e9..12e229e44 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -130,22 +130,27 @@ static int pack_objects(int fd, struct ref *refs, struct
sha1_array *extra, stru
return 0;
}
-static int receive_status(int in, struct ref *refs)
+static int receive_unpack_status(int in)
{
- struct ref *hint;
- int ret = 0;
- char *line = packet_read_line(in, NULL);
+ const char *line = packet_read_line(in, NULL);
if (!starts_with(line, "unpack "))
return error("did not receive remote status");
- if (strcmp(line, "unpack ok")) {
- error("unpack failed: %s", line + 7);
- ret = -1;
- }
+ if (strcmp(line, "unpack ok"))
+ return error("unpack failed: %s", line + 7);
+ return 0;
+}
+
+static int receive_status(int in, struct ref *refs)
+{
+ struct ref *hint;
+ int ret;
+
hint = NULL;
+ ret = receive_unpack_status(in);
while (1) {
char *refname;
char *msg;
- line = packet_read_line(in, NULL);
+ char *line = packet_read_line(in, NULL);
if (!line)
break;
if (!starts_with(line, "ok ") && !starts_with(line, "ng ")) {
--
2.12.0.429.gde83c8049
Re: [RFC 0/4] Shallow clones with on-demand fetch
On Mon, Mar 06, 2017 at 11:18:30AM -0800, Junio C Hamano wrote: > Mark Thomas writes: > > > This is a proof-of-concept, so it is in no way complete. It contains a > > few hacks to make it work, but these can be ironed out with a bit more > > work. What I have so far is sufficient to try out the idea. > > Two things that immediately come to mind (which may or may not be > real issues) are > > (1) What (if any) security model you have in mind. > > From object-confidentiality's point of view, this needs to be > enabled only on a host that allows > uploadpack.allowAnySHA1InWant but even riskier. > > From DoS point of view, you can make a short 40-byte request to > cause the other side emit megabytes of stuff. I do not think > it is a new problem (anybody can repeatedly request a clone of > large stuff), but there may be new ramifications. > > (2) If the interface to ask just one object kills the whole idea > due to roundtrip latency. > > You may want to be able to say "I want all objects reachable > from this tree; please give me a packfile of needed objects > assuming that I have all objects reachable from this other tree > (or these other trees)". Not just latency, but you also lose all of the benefits of delta compression. So if I asked for: git log -p -- foo.c and git is going to fault in all of the various versions of foo.c over time, it's _much_ more efficient to batch them into a single request, so that the server can reuse on-disk deltas between the various versions. That makes the transmission smaller, and it also makes it more likely for the server to be able to transmit the bits straight off the disk (rather than assembling each delta itself then zlib-compressing the result). Similarly, there's a latency tension in just finding out whether an object exists. When we call has_sha1_file() as part of a fetch, for example, we really want to be able to answer it quickly. So you'd probably want some mechanism to say "tell me the sha1, type, and size" of each object I _could_ get via upload-file. The size of that data is far from trivial for a large repository, but you're probably better off getting it once than paying the latency cost to fetch it piecemeal. -Peff
Re: [PATCH v3 0/9] Fix the early config
Hi Junio, On Fri, 3 Mar 2017, Junio C Hamano wrote: > 2/9 and corresponding 4/9 triggers "ERROR: trailing statements > should be on next line" from ../linux/scripts/checkpatch.pl because > of a line inherited from the original; I'll queue them with an > obvious style fix to work it around. Wow, it seems that script requires the entire Linux kernel repository to be cloned, you cannot simply download just the script and run it. In any case, as you pointed out, this style was inherited from the original. I squashed that style fix in, as you probably would have (your js/early-config does not have anything beyond v2.12.0). But I have to point out that it is conflating the purpose of this patch series (its goal is *not* to fix any style). I am absolutely not a fan of that. Ciao, Johannes
Re: [PATCH v5 1/1] config: add conditional include
On Mon, Mar 06, 2017 at 02:44:27PM -0800, Stefan Beller wrote:
> > +static int include_condition_is_true(const char *cond, size_t cond_len)
> > +{
> ...
> > +
> > + error(_("unrecognized include condition: %.*s"), (int)cond_len,
> > cond);
> > + /* unknown conditionals are always false */
> > + return 0;
> > +}
>
> Thanks for putting an error message here. I was looking at what
> is currently queued as origin/nd/conditional-config-include,
> which doesn't have this error() (yet / not any more?)
It's "not any more". It was in the original and I asked for it to be
removed during the last review.
> I'd strongly suggest to keep the error message here as that way
> a user can diagnose e.g. a typo in the condition easily.
>
> If we plan to extend this list of conditions in the future, and a user
> switches between versions of git, then they may see this message
> on a regular basis (whenever they use the 'old' version).
That would make it unlike the rest of the config-include mechanism
(which quietly ignores things it doesn't understand, like include.foo,
or include.foo.path), as well as the config code in general (which
ignores misspelt keys).
Some of that "quiet when you don't understand it" is historical
necessity. Older versions _can't_ complain about not knowing
include.path, because they don't yet know it's worth complaining about.
Likewise here, if this ships in v2.13 and a new condition "foo:" ships
in v2.14, you get:
v2.12 - no complaint; we don't even understand includeIf at all
v2.13 - complain; we know includeIf, but not "foo:"
v2.14 - works as expected
Which is kind of weird and inconsistent. But maybe the typo-detection
case is more important to get right than consistency across historical
versions.
-Peff
Re: regression: git push in non-shared repo stalls (v2.11.0+)
On Tue, 07 Mar 2017, Horst Schirmeier wrote: > I observe a regression that seems to have been introduced between > v2.10.0 and v2.11.0. When I try to push into a repository on the local > filesystem that belongs to another user and has not explicitly been > prepared for shared use, v2.11.0 shows some of the usual diagnostic > output and then freezes instead of announcing why it failed to push. Bisecting points to v2.10.1-373-g722ff7f: 722ff7f876c8a2ad99c42434f58af098e61b96e8 is the first bad commit commit 722ff7f876c8a2ad99c42434f58af098e61b96e8 Author: Jeff King Date: Mon Oct 3 16:49:14 2016 -0400 receive-pack: quarantine objects until pre-receive accepts -- PGP-Key 0xD40E0E7A
Re: [Request for Documentation] Differentiate signed (commits/tags/pushes)
Stefan Beller writes: > What is the difference between signed commits and tags? > (Not from a technical perspective, but for the end user) In addition to what the others already said: If you use GitHub, then in the web interface you get a "Verified" stamp for each signed commits: https://help.github.com/articles/signing-commits-using-gpg/ It's not a Git feature but a GitHub one, but given the popularity of GitHub, this probably led some users to believe that signed commits are more convenient than signed tags. -- Matthieu Moy http://www-verimag.imag.fr/~moy/
Reg : GSoC 2017 Microproject
Hi, I would like to participate in GSoC 2017 and I have chosen the Use unsigned integral type for collection of bits , idea from the Micro projects list. I request the help of the community for clarifying a few questions that I have. 1. Is this Microproject already taken ? 2. If it is free , I would like to point out one place where a signed int is used . In bisect.h , the structure struct rev_list_info uses flags of type signed int but , the value of MSB is not checked as a test case for any error checking. Hence it can be of type unsigned int. It is only used in rev-list.c for checking cases (BISECT_SHOW_ALL and REV_LIST_QUIET ). Is this a valid case. Thanks.
Re: [Request for Documentation] Differentiate signed (commits/tags/pushes)
On Mon, Mar 06, 2017 at 11:59:24AM -0800, Stefan Beller wrote: > What is the difference between signed commits and tags? > (Not from a technical perspective, but for the end user) I think git has never really tried to assign any _meaning_ to the signatures. It implements the technical bits and leaves it up to the user and their workflows to decide what a given signature means. People generally seem to take tag signatures to mean one (or both) of: 1. Certifying that the tree contents at that tag are the official, signed release contents (i.e., this is version 1.0, and I'm the maintainer, so believe me). 2. Certifying that all the history leading up to that tag is "official" in some sense (i.e., I'm the maintainer, and this was the tip of my git tree at the time I tagged the release). Signing individual commits _could_ convey meaning (2), but "all history leading up" part is unlikely to be something that the signer needs to enforce on every commit. In my opinion, the most useful meaning for commit-signing is simply to cryptographically certify the identity of the committer. We don't compare the GPG key ident to the "committer" field, but IMHO that would be a reasonable optional feature for verify-commit (I say optional because we're starting to assign semantics now). I think one of the reasons kernel (and git) developers aren't that interested in signed commits is that they're not really that interesting in a patch workflow. You're verifying the committer, who in this project is invariably Junio, and we just take his word that whatever is in the "author" field is reasonable. But for a project whose workflow is based around pushing and pulling commits, I think it does make sense. The author field may not always match the committer (e.g., in a cherry-pick), but it still lets you trace that attestation of the author back to the committer. And it's up to UI to make that distinction clear (e.g., if you push a signed cherry-pick to GitHub, the commit is labeled with something like "A U Thor committed with C O Mitter", and then you get a little "Verified" tag for C O Mitter that gives you more information about the signature). So I don't think it's just a checkbox feature. It's a useful thing for certain workflows that really want to be able to attribute individual commits with cryptographic strength. -Peff
Re: Delta compression not so effective
> Marius Storm-Olsen hat am 4. März 2017 um 09:27 > geschrieben: [...] > I really don't want the files on the mailinglist, so I'll send you a > link directly. However, small snippets for public discussions about > potential issues would be fine, obviously. git fast-export can anonymize a repository [1]. Maybe an anonymized repository still shows the issue you are seeing. [1]: https://www.git-scm.com/docs/git-fast-export#_anonymizing
Re: RFC: Another proposed hash function transition plan
On Mon, Mar 06, 2017 at 10:39:49AM -0800, Jonathan Tan wrote: > The "nohash" thing was in the hope of requiring only one signature to sign > all the hashes (in all the functions) that the user wants, while preserving > round-tripping ability. Thanks, this explained it very well. I understand the tradeoff now, though I am still of the opinion that simplicity is probably a more important goal. In practice I'd imagine that anybody doing commit-signing would just sign the more-secure hash, and people doing tag releases would probably do a dual-sign to be verifiable by both old and new clients. Those are infrequent enough that the extra computation probably doesn't matter. But that's just my gut feeling. -Peff
