* Re: Serious bug with Git-2.11.0-64-bit and Git-LFS
From: Nick Warr @ 2016-12-08 14:00 UTC (permalink / raw)
To: Lars Schneider; +Cc: git
In-Reply-To: <06520F42-BD49-4349-83B3-74DCA1E260CD@gmail.com>
That looks pretty much like the error we're dealing with, any reason
why going back a point version on Git (not git-lfs) would resolve the
issue however?
On 8 December 2016 at 13:57, Lars Schneider <larsxschneider@gmail.com> wrote:
>
>> On 08 Dec 2016, at 12:46, Nick Warr <nick.warr@bossastudios.com> wrote:
>>
>> Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with
>> 1.5.3) cloning from our locally hosted gitlab CE server via HTTPS.
>>
>> When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the
>> first time the clone will finish the checkout of the git part, then
>> when it starts downloading the LFS parts it will reliably finish with
>> a smudge filter error.
>>
>> This leaves the repo in an unstable condition, you can then fetch the
>> lfs part without issue, but checking out the lfs files or trying a git
>> reset --hard will continue to spit out the same error. As you can see,
>> the actual error is not particularly useful.
>>
>> fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi
>> Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs
>> failed
>> Unknown command ""
>>
>> Possibly it's due to the file extension being all capital letters, we
>> did manage to change the error by recommitting the file with a
>> lowercase extension, but it failed on the next file (which also had a
>> capital letter extension).
>>
>> This has happened on multiple fresh windows 10 64 bit installs,
>> different machines and target directories (to hopefully remove the
>> possibility of file permissions) where cloning is taking place.
>>
>> The solution is to back level to Git 2.10.2 and the error disappears.
>>
>> More than willing to provide any further information,
>
> Hi Nick,
>
> debris_junk.FBX is not stored properly in Git LFS.
> I explained the problem in detail here:
> https://github.com/git-lfs/git-lfs/issues/1729
>
> You should add the file properly to GitLFS to fix the problem.
> However, I think this is a regression in GitLFS and I hope it will
> be fixed in the next version.
>
> No change/fix in Git is required.
>
> Cheers,
> Lars
^ permalink raw reply
* Re: Serious bug with Git-2.11.0-64-bit and Git-LFS
From: Lars Schneider @ 2016-12-08 13:57 UTC (permalink / raw)
To: Nick Warr; +Cc: git
In-Reply-To: <CABW+60x0PSw7uNQZg4SeN7EAbNpraR_HWvnVFz1-fVLYX=B8RQ@mail.gmail.com>
> On 08 Dec 2016, at 12:46, Nick Warr <nick.warr@bossastudios.com> wrote:
>
> Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with
> 1.5.3) cloning from our locally hosted gitlab CE server via HTTPS.
>
> When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the
> first time the clone will finish the checkout of the git part, then
> when it starts downloading the LFS parts it will reliably finish with
> a smudge filter error.
>
> This leaves the repo in an unstable condition, you can then fetch the
> lfs part without issue, but checking out the lfs files or trying a git
> reset --hard will continue to spit out the same error. As you can see,
> the actual error is not particularly useful.
>
> fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi
> Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs
> failed
> Unknown command ""
>
> Possibly it's due to the file extension being all capital letters, we
> did manage to change the error by recommitting the file with a
> lowercase extension, but it failed on the next file (which also had a
> capital letter extension).
>
> This has happened on multiple fresh windows 10 64 bit installs,
> different machines and target directories (to hopefully remove the
> possibility of file permissions) where cloning is taking place.
>
> The solution is to back level to Git 2.10.2 and the error disappears.
>
> More than willing to provide any further information,
Hi Nick,
debris_junk.FBX is not stored properly in Git LFS.
I explained the problem in detail here:
https://github.com/git-lfs/git-lfs/issues/1729
You should add the file properly to GitLFS to fix the problem.
However, I think this is a regression in GitLFS and I hope it will
be fixed in the next version.
No change/fix in Git is required.
Cheers,
Lars
^ permalink raw reply
* [PATCH v2] commit: make --only --allow-empty work without paths
From: Andreas Krey @ 2016-12-08 13:50 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <xmqqd1h63xqn.fsf@gitster.mtv.corp.google.com>
--only is implied when paths are present, and required
them unless --amend. But with --allow-empty it should
be allowed as well - it is the only way to create an
empty commit in the presence of staged changes.
Also remove the post-fact cleverness indication;
it's in the man page anyway.
Signed-off-by: Andreas Krey <a.krey@gmx.de>
---
Ok, I've removed the clever message, as Junio suggested.
I don't know what else to do to make it acceptable. :-)
We're going to deploy it internally anyway, but I think
it belongs in git.git as well (aka 'Can I has "will queue"?').
Documentation/git-commit.txt | 3 ++-
builtin/commit.c | 4 +---
t/t7501-commit.sh | 9 +++++++++
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f2ab0ee2e..4f8f20a36 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -265,7 +265,8 @@ FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].)
If this option is specified together with `--amend`, then
no paths need to be specified, which can be used to amend
the last commit without committing changes that have
- already been staged.
+ already been staged. If used together with `--allow-empty`
+ paths are also not required, and an empty commit will be created.
-u[<mode>]::
--untracked-files[=<mode>]::
diff --git a/builtin/commit.c b/builtin/commit.c
index 8976c3d29..276c74034 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1206,10 +1206,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
if (also + only + all + interactive > 1)
die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
- if (argc == 0 && (also || (only && !amend)))
+ if (argc == 0 && (also || (only && !amend && !allow_empty)))
die(_("No paths with --include/--only does not make sense."));
- if (argc == 0 && only && amend)
- only_include_assumed = _("Clever... amending the last one with dirty index.");
if (argc > 0 && !also && !only)
only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths...");
if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d84897a67..0d8d89309 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -155,6 +155,15 @@ test_expect_success 'amend --only ignores staged contents' '
git diff --exit-code
'
+test_expect_success 'allow-empty --only ignores staged contents' '
+ echo changed-again >file &&
+ git add file &&
+ git commit --allow-empty --only -m "empty" &&
+ git cat-file blob HEAD:file >file.actual &&
+ test_cmp file.expect file.actual &&
+ git diff --exit-code
+'
+
test_expect_success 'set up editor' '
cat >editor <<-\EOF &&
#!/bin/sh
--
2.11.0.10.g1e1b186.dirty
^ permalink raw reply related
* Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR
From: Johannes Schindelin @ 2016-12-08 11:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Robbie Iannucci
In-Reply-To: <20161207194105.25780-4-gitster@pobox.com>
Hi Junio,
On Wed, 7 Dec 2016, Junio C Hamano wrote:
> The "libify sequencer" topic stopped passing the die_on_error option
> to hold_locked_index(), and this lost an error message from "git
> merge --ff-only $commit" when there are competing updates in
> progress.
Sorry for the breakage.
When libifying the code, I tried to be careful to retain the error
messages when not dying, and mistakenly assumed that hold_locked_index()
would do the same.
Ciao,
Dscho
^ permalink raw reply
* Serious bug with Git-2.11.0-64-bit and Git-LFS
From: Nick Warr @ 2016-12-08 11:46 UTC (permalink / raw)
To: git
Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with
1.5.3) cloning from our locally hosted gitlab CE server via HTTPS.
When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the
first time the clone will finish the checkout of the git part, then
when it starts downloading the LFS parts it will reliably finish with
a smudge filter error.
This leaves the repo in an unstable condition, you can then fetch the
lfs part without issue, but checking out the lfs files or trying a git
reset --hard will continue to spit out the same error. As you can see,
the actual error is not particularly useful.
fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi
Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs
failed
Unknown command ""
Possibly it's due to the file extension being all capital letters, we
did manage to change the error by recommitting the file with a
lowercase extension, but it failed on the next file (which also had a
capital letter extension).
This has happened on multiple fresh windows 10 64 bit installs,
different machines and target directories (to hopefully remove the
possibility of file permissions) where cloning is taking place.
The solution is to back level to Git 2.10.2 and the error disappears.
More than willing to provide any further information,
Nick Warr
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Johannes Sixt @ 2016-12-08 11:32 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git, sbeller, peff, jacob.keller
In-Reply-To: <20161207222927.GB116201@google.com>
Am 07.12.2016 um 23:29 schrieb Brandon Williams:
> Instead of assuming root is "/"
> I'll need to extract what root is from an absolute path. Aside from
> what root looks like, do most other path constructs behave similarly in
> unix and windows? (like ".." and "." as examples)
Yes, .. and . work the same way, except that they cannot appear in the
\\server\share part. I also think that .. does not cancel these parts.
As long as you use is_absolute_path() and do not simplify path
components before offset_1st_component(), you should be on the safe side.
> Since I don't really have a windows machine to test things it might be
> slightly difficult to get everything correct quickly but hopefully we can
> get this working :)
I'll lend a hand, of course, as time permits.
-- Hannes
^ permalink raw reply
* Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
From: Duy Nguyen @ 2016-12-08 11:05 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <20161208000357.GJ116201@google.com>
On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/07, Duy Nguyen wrote:
>> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
>> > Convert 'create_simplify()' to use the pathspec struct interface from
>> > using the '_raw' entry in the pathspec.
>>
>> It would be even better to kill this create_simplify() and let
>> simplify_away() handle struct pathspec directly.
>>
>> There is a bug in this code, that might have been found if we
>> simpify_away() handled pathspec directly: the memcmp() in
>> simplify_away() will not play well with :(icase) magic. My bad. If
>> :(icase) is used, the easiest/safe way is simplify nothing. Later on
>> maybe we can teach simplify_away() to do strncasecmp instead. We could
>> ignore exclude patterns there too (although not excluding is not a
>> bug).
>
> So are you implying that the simplify struct needs to be killed? That
> way the pathspec struct itself is being passed around instead?
Yes. simplify struct was a thing when pathspec was an array of char *.
At this point I think it can retire (when we have time to retire it)
--
Duy
^ permalink raw reply
* Re: [PATCH 01/17] mv: convert to using pathspec struct interface
From: Duy Nguyen @ 2016-12-08 11:04 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <20161208003604.GK116201@google.com>
On Thu, Dec 8, 2016 at 7:36 AM, Brandon Williams <bmwill@google.com> wrote:
>> > @@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix,
>> > {
>> > int i;
>> > const char **result;
>> > + struct pathspec ps;
>> > ALLOC_ARRAY(result, count + 1);
>> > - COPY_ARRAY(result, pathspec, count);
>> > - result[count] = NULL;
>> > +
>> > + /* Create an intermediate copy of the pathspec based on the flags */
>> > for (i = 0; i < count; i++) {
>> > - int length = strlen(result[i]);
>> > + int length = strlen(pathspec[i]);
>> > int to_copy = length;
>> > + char *it;
>> > while (!(flags & KEEP_TRAILING_SLASH) &&
>> > - to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
>> > + to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
>> > to_copy--;
>> > - if (to_copy != length || flags & DUP_BASENAME) {
>> > - char *it = xmemdupz(result[i], to_copy);
>> > - if (flags & DUP_BASENAME) {
>> > - result[i] = xstrdup(basename(it));
>> > - free(it);
>> > - } else
>> > - result[i] = it;
>> > - }
>> > +
>> > + it = xmemdupz(pathspec[i], to_copy);
>> > + if (flags & DUP_BASENAME) {
>> > + result[i] = xstrdup(basename(it));
>> > + free(it);
>> > + } else
>> > + result[i] = it;
>> > + }
>> > + result[count] = NULL;
>> > +
>> > + parse_pathspec(&ps,
>> > + PATHSPEC_ALL_MAGIC &
>> > + ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
>> > + PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,
>> > + prefix, result);
>> > + assert(count == ps.nr);
>> > +
>> > + /* Copy the pathspec and free the old intermediate strings */
>> > + for (i = 0; i < count; i++) {
>> > + const char *match = xstrdup(ps.items[i].match);
>> > + free((char *) result[i]);
>> > + result[i] = match;
>>
>> Sigh.. it looks so weird that we do all the parsing (in a _copy_
>> pathspec function) then remove struct pathspec and return the plain
>> string. I guess we can't do anything more until we rework cmd_mv code
>> to handle pathspec natively.
>>
>> At the least I think we should rename this function to something else.
>> But if you have time I really wish we could kill this function. I
>> haven't stared at cmd_mv() long and hard, but it looks to me that we
>> combining two separate functionalities in the same function here.
>>
>> If "mv" takes n arguments, then the first <n-1> arguments may be
>> pathspec, the last one is always a plain path. The "dest_path =
>> internal_copy_pathspec..." could be as simple as "dest_path =
>> prefix_path(argv[argc - 1])". the special treatment for this last
>> argument [1] can live here. Then, we can do parse_pathspec for the
>> <n-1> arguments in cmd_mv(). It's still far from perfect, because
>> cmd_mv can't handle pathspec properly, but it reduces the messy mess
>> in internal_copy_pathspec a bit, I hope.
>>
>
> Actually, after looking at this a bit more it seems like we could
> technically use prefix_path for both source and dest (based on how the
> current code is structured) since the source's provied must all exist (as
> in no wildcards are allowed). We could drop using the pathspec struct
> completely in addition to renaming the function (to what I'm still
> unsure).
Yeah that sounds good too (with a caveat: I'm not a heavy user of
git-mv nor touching this code a lot, I might be missing something).
It'll take some looong time before somebody starts converting it to
use pathspec properly, I guess. prefix_path() would keep the code
clean meanwhile.
> I agree that this code should probably be rewritten and made a
> bit cleaner, I don't know if that fits in the scope of this series or
> should be done as a followup patch. If you think it fits here then I
> can try and find some time to do the rework.
--
Duy
^ permalink raw reply
* Re: [PATCHv5 5/5] submodule: add embed-git-dir function
From: Duy Nguyen @ 2016-12-08 11:01 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, Git Mailing List, Junio C Hamano
In-Reply-To: <20161207210157.18932-6-sbeller@google.com>
On Thu, Dec 8, 2016 at 4:01 AM, Stefan Beller <sbeller@google.com> wrote:
> +/*
> + * Migrate the git directory of the given `path` from `old_git_dir` to
> + * `new_git_dir`. If an error occurs, append it to `err` and return the
> + * error code.
> + */
> +int relocate_gitdir(const char *path, const char *old_git_dir,
> + const char *new_git_dir, const char *displaypath,
> + struct strbuf *err)
> +{
> + int ret = 0;
> +
> + printf("Migrating git directory of '%s' from\n'%s' to\n'%s'\n",
> + displaypath, old_git_dir, new_git_dir);
I'm going to nag you about _() until your fingers automatically type
"_(" after "(" :-D
--
Duy
^ permalink raw reply
* Re: [PATCHv6 7/7] submodule: add absorb-git-dir function
From: Duy Nguyen @ 2016-12-08 10:56 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, Git Mailing List, Junio C Hamano
In-Reply-To: <20161208014623.7588-8-sbeller@google.com>
On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@google.com> wrote:
> diff --git a/dir.c b/dir.c
> index 8b74997c66..cc5729f733 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2774,3 +2774,15 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
> free(real_work_tree);
> free(real_git_dir);
> }
> +
> +/*
> + * Migrate the git directory of the given path from old_git_dir to new_git_dir.
> + */
> +void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
> +{
> + if (rename(old_git_dir, new_git_dir) < 0)
> + die_errno(_("could not migrate git directory from '%s' to '%s'"),
> + old_git_dir, new_git_dir);
> +
> + connect_work_tree_and_git_dir(path, new_git_dir);
> +}
Thank you!
--
Duy
^ permalink raw reply
* Re: [PATCHv6 5/7] worktree: add function to check if worktrees are in use
From: Duy Nguyen @ 2016-12-08 10:51 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, Git Mailing List, Junio C Hamano
In-Reply-To: <CACsJy8ANNz6FsJ4_5MOhj2Qqd+wHHu5UpVOAobqEiHU2KM26eg@mail.gmail.com>
On Thu, Dec 8, 2016 at 5:40 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> Alternatively, we could add a new flag to get_worktrees() to tell it
> to return all worktrees if there is a least one linked worktree, or
> return NULL if there's only main worktree. I'm not sure if this is
> clever or very stupid.
No, this may be better. Add a flag to say "returns linked worktrees
_only_". Which means when you're in a "normal" repo, get_worktrees()
with this flag returns NULL. When you're in a multiple-worktree repo,
it returns all linked worktrees (no main worktree). I think I might
have a use for this flag in addition to this uses_worktrees() here.
uses_worktrees() look quite simple with that flag
int uses_worktrees(void)
{
struct worktree **worktrees = get_worktrees(WT_LINKED_ONLY);
int retval = worktrees != NULL;
free_worktrees(worktrees);
return retval;
}
--
Duy
^ permalink raw reply
* Re: [PATCHv6 5/7] worktree: add function to check if worktrees are in use
From: Duy Nguyen @ 2016-12-08 10:40 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, Git Mailing List, Junio C Hamano
In-Reply-To: <20161208014623.7588-6-sbeller@google.com>
On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@google.com> wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> worktree.c | 24 ++++++++++++++++++++++++
> worktree.h | 7 +++++++
> 2 files changed, 31 insertions(+)
>
> diff --git a/worktree.c b/worktree.c
> index 75db689672..2559f33846 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -406,3 +406,27 @@ const struct worktree *find_shared_symref(const char *symref,
>
> return existing;
> }
> +
> +static int uses_worktree_internal(struct worktree **worktrees)
> +{
> + int i;
> + for (i = 0; worktrees[i]; i++)
> + ; /* nothing */
> +
> + free_worktrees(worktrees);
Ayy.. caller allocates, callee frees. This might become a new
maintenance nightmare. Elsewhere I believe we (Junio and me) discussed
the possibility of returning the number of worktrees from
get_worktrees() too. get_worktrees() would take an "int *", if not
NULL, we return the number of worktrees in that pointer.
It's probably a better approach, although I'm afraid it'll add a bit
more work on you.
Alternatively, we could add a new flag to get_worktrees() to tell it
to return all worktrees if there is a least one linked worktree, or
return NULL if there's only main worktree. I'm not sure if this is
clever or very stupid.
> + return i > 1;
> +}
> +
> +int uses_worktrees(void)
"has" may be a better verb than "uses". maybe "has_linked_worktrees"
since we always have and use the main worktree.
> +{
> + return uses_worktree_internal(get_worktrees(0));
> +}
> +
> +int submodule_uses_worktrees(const char *path)
> +{
> + struct worktree **worktrees = get_submodule_worktrees(path, 0);
> + if (!worktrees)
> + return 0;
> +
> + return uses_worktree_internal(worktrees);
> +}
> diff --git a/worktree.h b/worktree.h
> index 157fbc4a66..76027b1fd2 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -33,6 +33,13 @@ extern struct worktree **get_worktrees(unsigned flags);
> extern struct worktree **get_submodule_worktrees(const char *path,
> unsigned flags);
>
> +/*
> + * Returns 1 if more than one worktree exists.
> + * Returns 0 if only the main worktree exists.
> + */
> +extern int uses_worktrees(void);
> +extern int submodule_uses_worktrees(const char *path);
> +
> /*
> * Return git dir of the worktree. Note that the path may be relative.
> * If wt is NULL, git dir of current worktree is returned.
> --
> 2.11.0.rc2.30.gc512cbd.dirty
>
--
Duy
^ permalink raw reply
* Re: [PATCH 1/5] am: Fix filename in safe_to_abort() error message
From: Paul Tan @ 2016-12-08 10:21 UTC (permalink / raw)
To: Stephan Beyer
Cc: Git List, Christian Couder, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20161207215133.13433-1-s-beyer@gmx.net>
Hi Stephan,
On Thu, Dec 8, 2016 at 5:51 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> diff --git a/builtin/am.c b/builtin/am.c
> index 6981f42ce..7cf40e6f2 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)
>
> if (read_state_file(&sb, state, "abort-safety", 1) > 0) {
> if (get_oid_hex(sb.buf, &abort_safety))
> - die(_("could not parse %s"), am_path(state, "abort_safety"));
> + die(_("could not parse %s"), am_path(state, "abort-safety"));
Ah, this is obviously correct. Sorry for the oversight.
> } else
> oidclr(&abort_safety);
>
> --
> 2.11.0.27.g4eed97c
Thanks,
Paul
^ permalink raw reply
* Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly
From: Paul Tan @ 2016-12-08 10:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <CAGZ79kZHGqU2y19_uKhtVuE6vhspzPNpw-nVDnm8gLQ8u528kQ@mail.gmail.com>
Hi Junio,
On Thu, Dec 8, 2016 at 4:48 AM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Dec 7, 2016 at 11:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> The require_clean_work_tree() function calls hold_locked_index()
>> with die_on_error=0 to signal that it is OK if it fails to obtain
>> the lock, but unconditionally calls update_index_if_able(), which
>> will try to write into fd=-1.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
Ah, sorry about this. I was indeed misled by the function naming and
its comment ("do not complain if we can't"). Should have looked more
closely at the other call sites.
> However I think the promise of that function is
> to take care of the fd == -1?
Hmm, to add on, looking at the three other call sites of this
function, two of them (builtin/commit.c and builtin/describe.c)
basically do:
if (0 <= fd)
update_index_if_able(...)
with that 0 <= fd conditional. With this patch it becomes three out of
four. Perhaps the repeated use of this conditional is a sign that the
0 <= fd check could be built into update_index_if_able()? I think
there is precedent for building in these kind of checks --
rollback_lock_file() also does not fail if the lock file was not
successfully opened.
That said, the number of call sites is quite low so it's probably not
worth doing this.
Thanks,
Paul
^ permalink raw reply
* Re: [PATCHv6 4/7] worktree: get worktrees from submodules
From: Duy Nguyen @ 2016-12-08 10:09 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, Git Mailing List, Junio C Hamano
In-Reply-To: <20161208014623.7588-5-sbeller@google.com>
On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@google.com> wrote:
>
> worktree = xcalloc(1, sizeof(*worktree));
> worktree->path = strbuf_detach(&worktree_path, NULL);
> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
All the good stuff is outside context lines again.. Somewhere between
here we call add_head_info() which calls resolve_ref_unsafe(), which
always uses data from current repo, not the submodule we want it to
look at.
> @@ -167,7 +168,8 @@ static int compare_worktree(const void *a_, const void *b_)
> return fspathcmp((*a)->path, (*b)->path);
fspathcmp() reads core.ignorecase from current repo. I guess it's
insane to have this key different between repos on the same machine,
so it should be ok. But I want to point this out just in case I'm
missing something.
> @@ -188,7 +190,7 @@ struct worktree **get_worktrees(unsigned flags)
> if (is_dot_or_dotdot(d->d_name))
> continue;
>
> - if ((linked = get_linked_worktree(d->d_name))) {
> + if ((linked = get_linked_worktree(git_common_dir, d->d_name))) {
> ALLOC_GROW(list, counter + 1, alloc);
> list[counter++] = linked;
> }
> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)
> return list;
Right before this line is mark_current_worktree(), which in turn calls
get_git_dir() and not suitable for peeking into another repository the
way submodule code does. get_worktree_git_dir() called within that
function shares the same problem.
> }
>
> +struct worktree **get_worktrees(unsigned flags)
> +{
> + return get_worktrees_internal(get_git_common_dir(), flags);
> +}
> +
> +struct worktree **get_submodule_worktrees(const char *path, unsigned flags)
> +{
> + char *submodule_gitdir;
> + struct strbuf sb = STRBUF_INIT;
> + struct worktree **ret;
> +
> + submodule_gitdir = git_pathdup_submodule(path, "%s", "");
> + if (!submodule_gitdir)
> + return NULL;
> +
> + /* the env would be set for the superproject */
> + get_common_dir_noenv(&sb, submodule_gitdir);
Technically we need to read submodule_gitdir/.config and see if we can
understand core.repositoryformatversion, or find any unrecognized
extensions. But the problem is not specific to this code. And fixing
it is no small task. But perhaps we could call a dummy
validate_submodule_gitdir() here? Then when we implement that function
for real, we don't have to search the entire code base to see where to
put it.
Kinda off-topic though. Feel free to ignore the above comment.
> + ret = get_worktrees_internal(sb.buf, flags);
> +
> + strbuf_release(&sb);
> + free(submodule_gitdir);
> + return ret;
> +}
--
Duy
^ permalink raw reply
* Re: [PATCHv6 2/7] submodule helper: support super prefix
From: Duy Nguyen @ 2016-12-08 9:52 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, Git Mailing List, Junio C Hamano
In-Reply-To: <20161208014623.7588-3-sbeller@google.com>
On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@google.com> wrote:
> Just like main commands in Git, the submodule helper needs
> access to the superproject prefix. Enable this in the git.c
> but have its own fuse in the helper code by having a flag to
> turn on the super prefix.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> builtin/submodule--helper.c | 31 ++++++++++++++++++++-----------
> git.c | 2 +-
> 2 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 4beeda5f9f..33676a57cf 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1076,21 +1076,24 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
> return 0;
> }
>
> +#define SUPPORT_SUPER_PREFIX (1<<0)
> +
> struct cmd_struct {
> const char *cmd;
> int (*fn)(int, const char **, const char *);
> + int option;
unsigned int is probably safer for variables that are used as bit-flags.
> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> @@ -1100,9 +1103,15 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> die(_("submodule--helper subcommand must be "
> "called with a subcommand"));
>
> - for (i = 0; i < ARRAY_SIZE(commands); i++)
> - if (!strcmp(argv[1], commands[i].cmd))
> + for (i = 0; i < ARRAY_SIZE(commands); i++) {
> + if (!strcmp(argv[1], commands[i].cmd)) {
> + if (get_super_prefix() &&
> + !(commands[i].option & SUPPORT_SUPER_PREFIX))
> + die("%s doesn't support --super-prefix",
> + commands[i].cmd);
If it's meant for users to see, please _() the string.
> return commands[i].fn(argc - 1, argv + 1, prefix);
> + }
> + }
>
> die(_("'%s' is not a valid submodule--helper "
> "subcommand"), argv[1]);
> diff --git a/git.c b/git.c
> index efa1059fe0..98dcf6c518 100644
> --- a/git.c
> +++ b/git.c
> @@ -493,7 +493,7 @@ static struct cmd_struct commands[] = {
> { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
> { "stripspace", cmd_stripspace },
> - { "submodule--helper", cmd_submodule__helper, RUN_SETUP },
> + { "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},
The same macro defined twice in two separate .c files? Hmm.. it
confused me a bit because i thought there was a connection.. I guess
it's ok.
--
Duy
^ permalink raw reply
* Re: [PATCHv6 1/7] submodule: use absolute path for computing relative path connecting
From: Duy Nguyen @ 2016-12-08 9:45 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, Git Mailing List, Junio C Hamano
In-Reply-To: <20161208014623.7588-2-sbeller@google.com>
On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@google.com> wrote:
> The current caller of connect_work_tree_and_git_dir passes
> an absolute path for the `git_dir` parameter. In the future patch
> we will also pass in relative path for `git_dir`. Extend the functionality
> of connect_work_tree_and_git_dir to take relative paths for parameters.
>
> We could work around this in the future patch by computing the absolute
> path for the git_dir in the calling site, however accepting relative
> paths for either parameter makes the API for this function much harder
> to misuse.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> submodule.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 6f7d883de9..66c5ce5a24 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1227,23 +1227,25 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
> {
> struct strbuf file_name = STRBUF_INIT;
> struct strbuf rel_path = STRBUF_INIT;
> - const char *real_work_tree = xstrdup(real_path(work_tree));
> + char *real_git_dir = xstrdup(real_path(git_dir));
Is it a bad idea to rename the argument git_dir to git_dir_input, or
something, then have a new "git_dir" variable here? It certainly helps
reduce diff size, but not sure if the end result looks better or
worse.
> + char *real_work_tree = xstrdup(real_path(work_tree));
>
> /* Update gitfile */
> strbuf_addf(&file_name, "%s/.git", work_tree);
> write_file(file_name.buf, "gitdir: %s",
> - relative_path(git_dir, real_work_tree, &rel_path));
> + relative_path(real_git_dir, real_work_tree, &rel_path));
>
> /* Update core.worktree setting */
> strbuf_reset(&file_name);
> - strbuf_addf(&file_name, "%s/config", git_dir);
> + strbuf_addf(&file_name, "%s/config", real_git_dir);
> git_config_set_in_file(file_name.buf, "core.worktree",
> - relative_path(real_work_tree, git_dir,
> + relative_path(real_work_tree, real_git_dir,
> &rel_path));
>
> strbuf_release(&file_name);
> strbuf_release(&rel_path);
> - free((void *)real_work_tree);
> + free(real_work_tree);
> + free(real_git_dir);
> }
>
> int parallel_submodules(void)
> --
> 2.11.0.rc2.30.gc512cbd.dirty
>
--
Duy
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Duy Nguyen @ 2016-12-08 9:41 UTC (permalink / raw)
To: Brandon Williams
Cc: Stefan Beller, git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <20161205201619.GE68588@google.com>
On Tue, Dec 6, 2016 at 3:16 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/05, Stefan Beller wrote:
>> > static const char *real_path_internal(const char *path, int die_on_error)
>> > {
>> > - static struct strbuf sb = STRBUF_INIT;
>> > + static struct strbuf resolved = STRBUF_INIT;
>>
>> Also by having this static here, it is not quite thread safe, yet.
>>
>> By removing the static here we cannot do the early cheap check as:
>>
>> > /* We've already done it */
>> > - if (path == sb.buf)
>> > + if (path == resolved.buf)
>> > return path;
>>
>> I wonder how often we run into this case; are there some callers explicitly
>> relying on real_path_internal being cheap for repeated calls?
>> (Maybe run the test suite with this early return instrumented? Not sure how
>> to assess the impact of removing the cheap out return optimization)
>>
>> The long tail (i.e. the actual functionality) should actually be
>> faster, I'd imagine
>> as we do less than with using chdir.
>
> Depends on how expensive the chdir calls were. And I'm working to get
> rid of the static buffer. Just need have the callers own the memory
> first.
I suggest you turn this real_path_internal into strbuf_real_path. In
other words, it takes a strbuf and writes the result there, allocating
memory if needed.
This function can replace the two strbuf_addstr(..., real_path(..));
we have in setup.c and sha1_file.c. real_path() can own a static
strbuf buffer to retain current behavior. We could also have a new
wrapper real_pathdup() around strbuf_real_path(), which can replace 9
instances of xstrdup(real_path(...)) (and Stefan is adding a few more;
that's what led me back to these mails)
--
Duy
^ permalink raw reply
* Re: [PATCH 16/17] pathspec: small readability changes
From: Duy Nguyen @ 2016-12-08 9:23 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <20161207232724.GI116201@google.com>
On Thu, Dec 8, 2016 at 6:27 AM, Brandon Williams <bmwill@google.com> wrote:
>> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
>> > @@ -362,8 +368,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
>> > } else {
>> > item->original = xstrdup(elt);
>> > }
>> > - item->len = strlen(item->match);
>> > - item->prefix = prefixlen;
>> >
>> > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
>> > strip_submodule_slash_cheap(item);
>> > @@ -371,13 +375,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
>> > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
>> > strip_submodule_slash_expensive(item);
>> >
>> > - if (magic & PATHSPEC_LITERAL)
>> > + if (magic & PATHSPEC_LITERAL) {
>> > item->nowildcard_len = item->len;
>> > - else {
>> > + } else {
>> > item->nowildcard_len = simple_length(item->match);
>> > if (item->nowildcard_len < prefixlen)
>> > item->nowildcard_len = prefixlen;
>> > }
>> > +
>> > item->flags = 0;
>>
>> You probably can move this line up with the others too.
>
> I didn't move the item->flags assignment up since the code immediately
> following this assignment deal with setting item->flags. I made more
> sense to keep them grouped.
It's probably why I put it there in the beginning :) Yes let's leave
it where it is then.
--
Duy
^ permalink raw reply
* Re: [PATCH 11/17] pathspec: factor global magic into its own function
From: Duy Nguyen @ 2016-12-08 9:20 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <20161207223936.GD116201@google.com>
On Thu, Dec 8, 2016 at 5:39 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/07, Duy Nguyen wrote:
>> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
>> > Create helper functions to read the global magic environment variables
>> > in additon to factoring out the global magic gathering logic into its
>> > own function.
>> >
>> > Signed-off-by: Brandon Williams <bmwill@google.com>
>> > ---
>> > pathspec.c | 120 +++++++++++++++++++++++++++++++++++++------------------------
>> > 1 file changed, 74 insertions(+), 46 deletions(-)
>> >
>> > diff --git a/pathspec.c b/pathspec.c
>> > index 5afebd3..08e76f6 100644
>> > --- a/pathspec.c
>> > +++ b/pathspec.c
>> > @@ -87,6 +87,74 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
>> > strbuf_addf(sb, ",prefix:%d)", prefixlen);
>> > }
>> >
>> > +static inline int get_literal_global(void)
>> > +{
>> > + static int literal_global = -1;
>> > +
>> > + if (literal_global < 0)
>> > + literal_global = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT,
>> > + 0);
>>
>> These zeros look so lonely. I know it would exceed 80 columns if we
>> put it on the previous line. But I think it's ok for occasional
>> exceptions. Or you could rename noglob_global to noglob.
>
> I was thinking the same thing but was so torn between the char limit. I
> think it's probably ok to rename these vars by drooping the global since
> the function name themselves indicate they are global.
Exactly. I almost suggested just "ret" for that reason, but it was a
bit on the extreme side, relying entirely on the function's name for
context.
--
Duy
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Torsten Bögershausen @ 2016-12-08 7:55 UTC (permalink / raw)
To: Brandon Williams
Cc: Junio C Hamano, Ramsay Jones, git, sbeller, peff, jacob.keller
In-Reply-To: <20161207221335.GA116201@google.com>
On Wed, Dec 07, 2016 at 02:13:35PM -0800, Brandon Williams wrote:
> On 12/07, Junio C Hamano wrote:
> > Torsten Bögershausen <tboegi@web.de> writes:
> >
> > > But in any case it seems that e.g.
> > > //SEFVER/SHARE/DIR1/DIR2/..
> > > must be converted into
> > > //SEFVER/SHARE/DIR1
> > >
> > > and
> > > \\SEFVER\SHARE\DIR1\DIR2\..
> > > must be converted into
> > > \\SEFVER\SHARE\DIR1
> >
> > Additional questions that may be interesting are:
> >
> > //A/B/../C is it //A/C? is it an error?
Yes, at least under Windows.
If I have e.g. a Raspi with SAMBA, I can put a git Repository here:
//raspi/torsten/projects/git
If I use
git push //raspi/torsten/../junio/projects/git
that should be an error.
> > //A/B/../../C/D is it //C/D? is it an error?
> >
Same for
git push /raspi/../raspi2/torsten//projects/git
>
>
> Also is //.. the same as //? I would assume so since /.. is /
>
Under Windows //.. is simply illegal, I would say.
The documentation here
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
Mentions these 2 examples, what to feed into the WIN32 file API:
a)
\\?\D:\very-long-path
b)
\\server\share\path\file"
c)
"\\?\UNC\server\share\path"
So whatever we do, the ".." resoltion is only allowed to look at
"very-long-path" or "path".
Some conversion may be done in mingw.c:
https://github.com/github/git-msysgit/blob/master/compat/mingw.c
So what I understand, '/' in Git are already converted into '\' if needed ?
It seams that we may wnat a function get_start_of_path(uncpath),
which returns:
get_start_of_path_win("//?/D:/very-long-path") "/very-long-path"
get_start_of_path_win("//server/share/path/file") "/path/file"
get_start_of_path_win("//?/UNC/server/share/path") "/path"
(I don't know if we need the variant with '\', but is would'n hurt):
get_start_of_path_win("\\\\?\\D:\\very-long-path") "\\very-long-path"
get_start_of_path_win("\\\\server\\share\\path\\file") "\\path\\file"
get_start_of_path_win("\\\\?\\UNC\\server\\share\\path") "\\path"
Then the non-windows version could simply return
get_start_of_path_non_win(something) something
Does this make sense ?
^ permalink raw reply
* Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
From: Christian Couder @ 2016-12-08 7:50 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Junio C Hamano, git, SZEDER Gábor
In-Reply-To: <6facca6e-622a-ea8f-89d8-a18b7faee3cc@gmx.net>
Hi,
On Wed, Dec 7, 2016 at 7:36 PM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> On 12/06/2016 07:58 PM, Junio C Hamano wrote:
>
>> (1) The third invocation of "cherry-pick" with "--abort" to get rid
>> of the state from the unfinished cherry-pick we did previously
>> is necessary, but the command does not notice that we resetted
>> to a new branch AND we even did some other work there. This
>> loses end-user's work.
>>
>> "git cherry-pick --abort" should learn from "git am --abort"
>> that has an extra safety to deal with the above workflow. The
>> state from the unfinished "am" is removed, but the head is not
>> rewound to avoid losing end-user's work.
>>
>> You can try by replacing two instances of
>>
>> $ git cherry-pick 0582a34f52..a94bb68397
>>
>> with
>>
>> $ git format-patch --stdout 0582a34f52..a94bb68397 | git am
>>
>> in the above sequence, and conclude with "git am--abort" to see
>> how much more pleasant and safe "git am --abort" is.
> Definitely. I'd volunteer to add that safety guard. (But (2) remains.)
That would be great if you could take care of that!
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C
From: Pranit Bauva @ 2016-12-08 6:43 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <14b9ba3a-4568-2e4b-0b1a-f0ee75a7c677@gmx.net>
Hey Stephan,
On Wed, Dec 7, 2016 at 5:24 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi Pranit,
>
> On 12/06/2016 11:40 PM, Pranit Bauva wrote:
>> On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
>>> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>>>> +static int bisect_state(struct bisect_terms *terms, const char **argv,
>>>> + int argc)
>>>> +{
>>>> + const char *state = argv[0];
>>>> +
>>>> + get_terms(terms);
>>>> + if (check_and_set_terms(terms, state))
>>>> + return -1;
>>>> +
>>>> + if (!argc)
>>>> + die(_("Please call `--bisect-state` with at least one argument"));
>>>
>>> I think this check should move to cmd_bisect__helper. There are also the
>>> other argument number checks.
>>
>> Not really. After the whole conversion, the cmdmode will cease to
>> exists while bisect_state will be called directly, thus it is
>> important to check it here.
>
> Okay, that's a point.
> In that case, you should probably use "return error()" again and the
> message (mentioning "--bisect-state") does not make sense when
> --bisect-state ceases to exist.
True. Will change accordingly.
>>>> +
>>>> + if (argc == 1 && one_of(state, terms->term_good,
>>>> + terms->term_bad, "skip", NULL)) {
>>>> + const char *bisected_head = xstrdup(bisect_head());
>>>> + const char *hex[1];
>>>
>>> Maybe:
>>> const char *hex;
>>>
>>>> + unsigned char sha1[20];
>>>> +
>>>> + if (get_sha1(bisected_head, sha1))
>>>> + die(_("Bad rev input: %s"), bisected_head);
>>>
>>> And instead of...
>>>
>>>> + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
>>>> + return -1;
>>>> +
>>>> + *hex = xstrdup(sha1_to_hex(sha1));
>>>> + if (check_expected_revs(hex, 1))
>>>> + return -1;
>>>
>>> ... simply:
>>>
>>> hex = xstrdup(sha1_to_hex(sha1));
>>> if (set_state(terms, state, hex)) {
>>> free(hex);
>>> return -1;
>>> }
>>> free(hex);
>>>
>>> where:
>>
>> Yes I am planning to convert all places with hex rather than the sha1
>> but not yet, maybe in an another patch series because currently a lot
>> of things revolve around sha1 and changing its behaviour wouldn't
>> really be a part of a porting patch series.
>
> I was not suggesting a change of behavior, I was suggesting a simple
> helper function set_state() to get rid of code duplication above and
> some lines below:
>
>>> ... And replace this:
>>>
>>>> + const char **hex_string = (const char **) &hex.items[i].string;
>>>> + if(bisect_write(state, *hex_string, terms, 0)) {
>>>> + string_list_clear(&hex, 0);
>>>> + return -1;
>>>> + }
>>>> + if (check_expected_revs(hex_string, 1)) {
>>>> + string_list_clear(&hex, 0);
>>>> + return -1;
>>>> + }
>>>
>>> by:
>>>
>>> const char *hex_str = hex.items[i].string;
>>> if (set_state(terms, state, hex_string)) {
>>> string_list_clear(&hex, 0);
>>> return -1;
>>> }
>
> See?
I can do this change of using set_state() keeping aside the sha1 to
hex and such change.
>>>> @@ -184,8 +137,8 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
>>>> state="$TERM_GOOD"
>>>> fi
>>>>
>>>> - # We have to use a subshell because "bisect_state" can exit.
>>>> - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
>>>> + # We have to use a subshell because "--bisect-state" can exit.
>>>> + ( git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" )
>>>
>>> The new comment is funny, but you don't need a subshell here any
>>> longer.
>>
>> True, but right now I didn't want to modify that part of the source
>> code to remove the comment. I will remove the comment all together
>> when I port bisect_run()
> For most of the patches, I was commenting on the current state, not on
> the big picture.
>
> Still I think that it is better to remove the comment and the subshell
> instead of making the comment weird ("yes the builtin can exit, but why
> do we need a subshell?" vs "yes the shell function calls exit, so we
> need a subshell because we do not want to exit this shell script")
Sure I will remove the comment.
Regards,
Pranit Bauva
^ permalink raw reply
* [PATCHv6 6/7] move connect_work_tree_and_git_dir to dir.h
From: Stefan Beller @ 2016-12-08 1:46 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161208014623.7588-1-sbeller@google.com>
That function was primarily used by submodule code, but the function
itself is not inherently about submodules. In the next patch we'll
introduce relocate_git_dir, which can be used by worktrees as well,
so find a neutral middle ground in dir.h.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
dir.c | 26 ++++++++++++++++++++++++++
dir.h | 1 +
submodule.c | 26 --------------------------
submodule.h | 1 -
4 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/dir.c b/dir.c
index bfa8c8a9a5..8b74997c66 100644
--- a/dir.c
+++ b/dir.c
@@ -2748,3 +2748,29 @@ void untracked_cache_add_to_index(struct index_state *istate,
{
untracked_cache_invalidate_path(istate, path);
}
+
+/* Update gitfile and core.worktree setting to connect work tree and git dir */
+void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
+{
+ struct strbuf file_name = STRBUF_INIT;
+ struct strbuf rel_path = STRBUF_INIT;
+ char *real_git_dir = xstrdup(real_path(git_dir));
+ char *real_work_tree = xstrdup(real_path(work_tree));
+
+ /* Update gitfile */
+ strbuf_addf(&file_name, "%s/.git", work_tree);
+ write_file(file_name.buf, "gitdir: %s",
+ relative_path(real_git_dir, real_work_tree, &rel_path));
+
+ /* Update core.worktree setting */
+ strbuf_reset(&file_name);
+ strbuf_addf(&file_name, "%s/config", real_git_dir);
+ git_config_set_in_file(file_name.buf, "core.worktree",
+ relative_path(real_work_tree, real_git_dir,
+ &rel_path));
+
+ strbuf_release(&file_name);
+ strbuf_release(&rel_path);
+ free(real_work_tree);
+ free(real_git_dir);
+}
diff --git a/dir.h b/dir.h
index 97c83bb383..051674a431 100644
--- a/dir.h
+++ b/dir.h
@@ -335,4 +335,5 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);
void add_untracked_cache(struct index_state *istate);
void remove_untracked_cache(struct index_state *istate);
+extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
#endif
diff --git a/submodule.c b/submodule.c
index 66c5ce5a24..0bb50b4b62 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1222,32 +1222,6 @@ int merge_submodule(unsigned char result[20], const char *path,
return 0;
}
-/* Update gitfile and core.worktree setting to connect work tree and git dir */
-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
-{
- struct strbuf file_name = STRBUF_INIT;
- struct strbuf rel_path = STRBUF_INIT;
- char *real_git_dir = xstrdup(real_path(git_dir));
- char *real_work_tree = xstrdup(real_path(work_tree));
-
- /* Update gitfile */
- strbuf_addf(&file_name, "%s/.git", work_tree);
- write_file(file_name.buf, "gitdir: %s",
- relative_path(real_git_dir, real_work_tree, &rel_path));
-
- /* Update core.worktree setting */
- strbuf_reset(&file_name);
- strbuf_addf(&file_name, "%s/config", real_git_dir);
- git_config_set_in_file(file_name.buf, "core.worktree",
- relative_path(real_work_tree, real_git_dir,
- &rel_path));
-
- strbuf_release(&file_name);
- strbuf_release(&rel_path);
- free(real_work_tree);
- free(real_git_dir);
-}
-
int parallel_submodules(void)
{
return parallel_jobs;
diff --git a/submodule.h b/submodule.h
index d9e197a948..4e3bf469b4 100644
--- a/submodule.h
+++ b/submodule.h
@@ -65,7 +65,6 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c
int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,
struct string_list *needs_pushing);
int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
int parallel_submodules(void);
/*
--
2.11.0.rc2.30.gc512cbd.dirty
^ permalink raw reply related
* [PATCHv6 3/7] test-lib-functions.sh: teach test_commit -C <dir>
From: Stefan Beller @ 2016-12-08 1:46 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161208014623.7588-1-sbeller@google.com>
Specifically when setting up submodule tests, it comes in handy if
we can create commits in repositories that are not at the root of
the tested trash dir. Add "-C <dir>" similar to gits -C parameter
that will perform the operation in the given directory.
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/test-lib-functions.sh | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..579e812506 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -157,16 +157,21 @@ debug () {
GIT_TEST_GDB=1 "$@"
}
-# Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
+# Call test_commit with the arguments
+# [-C <directory>] <message> [<file> [<contents> [<tag>]]]"
#
# This will commit a file with the given contents and the given commit
# message, and tag the resulting commit with the given tag name.
#
# <file>, <contents>, and <tag> all default to <message>.
+#
+# If the first argument is "-C", the second argument is used as a path for
+# the git invocations.
test_commit () {
notick= &&
signoff= &&
+ indir= &&
while test $# != 0
do
case "$1" in
@@ -176,21 +181,26 @@ test_commit () {
--signoff)
signoff="$1"
;;
+ -C)
+ indir="$2"
+ shift
+ ;;
*)
break
;;
esac
shift
done &&
+ indir=${indir:+"$indir"/} &&
file=${2:-"$1.t"} &&
- echo "${3-$1}" > "$file" &&
- git add "$file" &&
+ echo "${3-$1}" > "$indir$file" &&
+ git ${indir:+ -C "$indir"} add "$file" &&
if test -z "$notick"
then
test_tick
fi &&
- git commit $signoff -m "$1" &&
- git tag "${4:-$1}"
+ git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
+ git ${indir:+ -C "$indir"} tag "${4:-$1}"
}
# Call test_merge with the arguments "<message> <commit>", where <commit>
--
2.11.0.rc2.30.gc512cbd.dirty
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox