* Re: [PATCH v2] date-formats.txt: Typo fix
From: Junio C Hamano @ 2016-11-28 18:06 UTC (permalink / raw)
To: Luis Ressel; +Cc: git
In-Reply-To: <20161125173657.9656-1-aranea@aixah.de>
Luis Ressel <aranea@aixah.de> writes:
> Last time I checked, I was living in the UTC+01:00 time zone. UTC+02:00
> would be Central European _Summer_ Time.
> ---
Thanks; please sign-off your patches, even a trivial ones like this.
> Documentation/date-formats.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/date-formats.txt b/Documentation/date-formats.txt
> index 35e8da2..6926e0a 100644
> --- a/Documentation/date-formats.txt
> +++ b/Documentation/date-formats.txt
> @@ -11,7 +11,7 @@ Git internal format::
> It is `<unix timestamp> <time zone offset>`, where `<unix
> timestamp>` is the number of seconds since the UNIX epoch.
> `<time zone offset>` is a positive or negative offset from UTC.
> - For example CET (which is 2 hours ahead UTC) is `+0200`.
> + For example CET (which is 1 hour ahead of UTC) is `+0100`.
>
> RFC 2822::
> The standard email format as described by RFC 2822, for example
^ permalink raw reply
* Re: [PATCH 31/35] pathspec: allow querying for attributes
From: Brandon Williams @ 2016-11-28 18:03 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, pclouds, git
In-Reply-To: <20161110203428.30512-32-sbeller@google.com>
On 11/10, Stefan Beller wrote:
> +static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)
> +{
>
> [...]
>
> + if (!item->attr_check)
> + git_attr_check_initv(&item->attr_check, attrs.argv);
> + else
> + die(_("Only one 'attr:' specification is allowed."));
> +
> + argv_array_clear(&attrs);
> + string_list_clear(&list, 0);
> + return;
> +}
Unnecessary return statement, maybe you want to remove it?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 31/35] pathspec: allow querying for attributes
From: Brandon Williams @ 2016-11-28 18:02 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, pclouds, git
In-Reply-To: <20161110203428.30512-32-sbeller@google.com>
On 11/10, Stefan Beller wrote:
> @@ -500,6 +586,18 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
>
> void clear_pathspec(struct pathspec *pathspec)
> {
> + int i, j;
> + for (i = 0; i < pathspec->nr; i++) {
> + if (!pathspec->items[i].attr_match_nr)
> + continue;
> + for (j = 0; j < pathspec->items[j].attr_match_nr; j++)
This looks like a bug. Should it be:
for (j = 0; j < pathspec->items[i].attr_match_nr; j++)
where items[j] -> items[i]
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] l10n: de.po: translate 210 new message
From: Ralf Thielow @ 2016-11-28 17:55 UTC (permalink / raw)
To: Jiang Xin
Cc: Git List, Thomas Rast, Jan Krüger, Christian Stimming,
Phillip Sz, Matthias Rüster, Magnus Görlitz
In-Reply-To: <CANYiYbEzoN8S0o7_1N4hpO6OHZqq5Y4cMPxPPLEMA4TJ2n-d1g@mail.gmail.com>
2016-11-28 15:21 GMT+01:00 Jiang Xin <worldhello.net@gmail.com>:
> 2016-11-25 2:25 GMT+08:00 Ralf Thielow <ralf.thielow@gmail.com>:
>> #: sequencer.c:251
>> -#, fuzzy, c-format
>> +#, c-format
>> msgid "could not write eol to '%s"
>
> Unmatched single quote has been fixed in l10n round 3.
> You can rebase and update de.po file.
>
> BTW, Git 2.11.0 will be released tomorrow, please send PR in time.
I've rebased, fixed the subject line and send a pull request to you.
Thanks
>
>> -msgstr "Konnte nicht nach '%s' schreiben."
>> +msgstr "Konnte EOL nicht nach '%s' schreiben."
>
> --
> Jiang Xin
^ permalink raw reply
* Re: [feature request] Make "commit --only" work with new files
From: Junio C Hamano @ 2016-11-28 17:52 UTC (permalink / raw)
To: Luis Ressel; +Cc: git
In-Reply-To: <20161125175619.19e13e59@gentp.lnet>
Luis Ressel <aranea@aixah.de> writes:
> currently "git commit --only <file>" only works if <file> is already
> checked into the repo, but not with newly created and still untracked
> files (builtin/commit.c:list_path() throws the error "error: pathspec
> '<file>' did not match any file(s) known to git.")
The fact that pathspec on the command line of "commit" does not let
you add new files is true with or without "--only". Yes, "--only"
is the default so with or without it it means the same thing, but
even with "--include" that says "I am happy with what is in the
index, but please take further changes to these paths, too" does not
affect files that are not so far tracked.
> I don't think this limitation is intented.
This actually was intended. Back when "commit [--opts] <pathspec>"
was invented, out tools were designed to avoid adding unwanted files
by mistake (e.g. "update-index" without an explicit "--add" work
only on paths already known to Git), and the behaviour is in line
with that design. It partly was because back then we didn't even
have ".gitignore" mechanism, I would say. So it was not only
intended, but was a sensible design decision back then.
I suspect that an argument could be made that it is about time we
shift the design philosophy and allow adding new paths with pathspec
given to "git commit". If I were designing Git without any existing
users, with all the other goodies we already have, and "git commit"
in my version of Git lacked pathspec support now, I might allow it
to add untracked files with the pathspec [*1*].
There however are backward compatibility worries. People who are
used to the designed behaviour for the past 10 years still expect
and rely on that
$ git commit <path-to-dir>
to take _only_ changes to the files that are already tracked in the
<path-to-dir> since the last "git add" they did to them, and other
files in the same <path-to-dir> that are not yet ready (and they
deliberately left un-added) will not be in the commit.
[Footnote]
*1* I might decide not to, after thinking long enough, though. The
point is that times changed and the trade off between safetly of
not adding at the point of commit and convenience of adding has
shifted. I haven't thought enough to decide that the shift is
big enough to warrant the change in behaviour, but at least it
is now worth considering.
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2016-11-28 17:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <xmqqa8cjlekl.fsf@gitster.mtv.corp.google.com>
Hi,
On Mon, 28 Nov 2016, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Sat, Nov 26, 2016 at 02:01:36PM +0100, Johannes Schindelin wrote:
> >
> >> > If you want to control it from outside the test script, you'd need
> >> > something like:
> >> >
> >> > if test "$GIT_TEST_DIFFTOOL" = "builtin"
> >>
> >> That is a bit magic. I first used "GIT_USE_BUILTIN_DIFFTOOL" and it did
> >> not work. My name is arguably more correct (see also Jakub's note about
> >> "naming is hard"), but yours works because there is a "TEST" substring in
> >> it.
> >
> > Yes. You are free to add an exception to the env list in test-lib.sh,
> > but we usually use GIT_TEST_* to avoid having to do so.
>
> Perhaps
>
> - The switch between "do I use builtin, or scripted?" mechanism in
> 1/2 can look at an environment (just like the old "am" rewrite
> series did), instead of configuration. This would make the code
> a lot more simppler (you do not have to worry about the
> interaction between "setup" and .git/config).
>
> - That environment variable can be named GIT_TEST_BUILTIN_DIFFTOOL;
> after all, people are opting into helping to test the new shiny
> to make/prove it ready sooner.
>
> - The bulk of the existing test for difftool can be moved to a
> dot-included file (in a way similar to t/annotate-tests are
> usable to test both annotate and blame-imitating-annotate).
> Existing PERL prerequisites can all be lost.
>
> - Two tests can include that dot-included file; one would
> explicitly unset that environment (and gives up without PERL
> prerequisite), while the other explicitly sets it.
If my main worry was the test suite, I would agree with this plan.
However, I have been bitten time and again by problems that occurred only
in production, our test suite (despite taking already waaaaaay too long to
be truly useful in my daily development) was simply not good enough.
So my plan was different: to let end users opt-in to test this new beast
thoroughly, more thoroughly than any review would.
And for that, environment variables are just not an option. I need
something that can be configured in a portable application, so that the
main Git for Windows installation is unaffected.
My original "create a file in libexec/git-core/" was simple, did the job
reliably, and worked also for testing.
It is a pity that you two gentlemen shot it down for being inelegant. And
ever since, we try to find a solution that is as simple, works as
reliably, also for testing, *and* appeases your tastes.
Ciao,
Dscho
^ permalink raw reply
* Re: trustExitCode doesn't apply to vimdiff mergetool
From: Junio C Hamano @ 2016-11-28 17:17 UTC (permalink / raw)
To: David Aguilar; +Cc: Jeff King, Dun Peal, Git ML
In-Reply-To: <20161128021554.GA30863@gmail.com>
David Aguilar <davvid@gmail.com> writes:
> deltawalker, diffmerge, emerge, kdiff3, kompare, and tkdiff originally
> provided behavior that matched trustExitCode=true.
>
> The default for all tools is trustExitCode=false, which conflicts with
> these tools' defaults. Allow tools to advertise their own default value
> for trustExitCode so that users do not need to opt-in to the original
> behavior.
>
> While this makes the default inconsistent between tools, it can still be
> overridden, and it makes it consistent with the current Git behavior.
I think this is sensible, because the way I look at this issue is
that in an ideal world, we would want all tool backends consistently
give us usable exit codes, but some tools are known to give unusable
exit codes, so we ignore their exit codes by default.
As to the implementation, I think you can reduce the duplication by
having each tool backend
- export a new function that echos "true" or "false"; or
- export a new function that returns true or false; or
- set a variable whose value is either "true" or "false"
and use that from the trust_exit_code() in git-mergetool--lib.sh.
Something like this (for the second alternative).
trust_exit_code () {
if git config --bool "mergtool.$.trustExitCode"
then
:; # OK
elif mergetool_exitcode_trustable
then
echo true
else
echo false
fi
}
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Junio C Hamano @ 2016-11-28 17:06 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <20161127165058.uxujjehyjq7httro@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sat, Nov 26, 2016 at 02:01:36PM +0100, Johannes Schindelin wrote:
>
>> > If you want to control it from outside the test script, you'd need
>> > something like:
>> >
>> > if test "$GIT_TEST_DIFFTOOL" = "builtin"
>>
>> That is a bit magic. I first used "GIT_USE_BUILTIN_DIFFTOOL" and it did
>> not work. My name is arguably more correct (see also Jakub's note about
>> "naming is hard"), but yours works because there is a "TEST" substring in
>> it.
>
> Yes. You are free to add an exception to the env list in test-lib.sh,
> but we usually use GIT_TEST_* to avoid having to do so.
Perhaps
- The switch between "do I use builtin, or scripted?" mechanism in
1/2 can look at an environment (just like the old "am" rewrite
series did), instead of configuration. This would make the code
a lot more simppler (you do not have to worry about the
interaction between "setup" and .git/config).
- That environment variable can be named GIT_TEST_BUILTIN_DIFFTOOL;
after all, people are opting into helping to test the new shiny
to make/prove it ready sooner.
- The bulk of the existing test for difftool can be moved to a
dot-included file (in a way similar to t/annotate-tests are
usable to test both annotate and blame-imitating-annotate).
Existing PERL prerequisites can all be lost.
- Two tests can include that dot-included file; one would
explicitly unset that environment (and gives up without PERL
prerequisite), while the other explicitly sets it.
^ permalink raw reply
* Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c
From: Junio C Hamano @ 2016-11-28 16:56 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <CAP8UFD0Q4yTfnhLW6XbfqbxBvMc_QmZTEn4XJb-9fj6Uvq6hkw@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> What about something like this then:
>
> /* This dies if the configured or default date is in the future */
> extern int git_config_get_expiry_or_die(const char *key, const char **output);
>
> Also git_config_get_int(), git_config_get_bool() and probably other
> such functions are indirectly calling git_config_int() which die()s is
> the value is not a number, so it feels a bit strange to have only one
> function with a name that ends with "_or_die" when many other
> functions could die(). In fact it could give a false sense of security
> to those who just read cache.h.
It is half-a-good-point that existing functions would die and I
agree with you that get_expiry_or_die can lose _or_die part.
But get_int() dying when it is fed something that is not "int" (or
something that cannot be returned in "int") is one thing; get_date()
being happy when it is fed a string that is a valid date 2015-01-01
but dying when fed another valid date 2018-01-01 is quite puzzling.
I think get_expiry() is OK.
Q: What's expiry? How is it different from date
A: A past timestamp that is speifies the cutoff time for the purpose
of expiring old stuff. You can say "now" to expire everything
you have.
is more pleasant and understandable conversation than
Q: Why can I feed only past date to get_date()? Otherwise it dies.
A: ...? The people who named the function didn't know what "date" was?
^ permalink raw reply
* Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c
From: Christian Couder @ 2016-11-28 16:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <xmqqlgwam76c.fsf@gitster.mtv.corp.google.com>
On Wed, Nov 23, 2016 at 6:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> Ok it will appear like this in cache.h:
>>
>> /* This dies if the configured or default date is in the future */
>> extern int git_config_get_expire_date_string(const char *key, const
>> char **output);
>
> Those who imitate existing callsites never read comments, and you
> need to spend effort to get the name right to protect the codebase
> from them.
>
> "get-expiry" may be shorter. Neither still does not say it will
> die, though.
What about something like this then:
/* This dies if the configured or default date is in the future */
extern int git_config_get_expiry_or_die(const char *key, const char **output);
Also git_config_get_int(), git_config_get_bool() and probably other
such functions are indirectly calling git_config_int() which die()s is
the value is not a number, so it feels a bit strange to have only one
function with a name that ends with "_or_die" when many other
functions could die(). In fact it could give a false sense of security
to those who just read cache.h.
^ permalink raw reply
* Re: cherry-pick -Xrenormalize fails with formerly CRLF files
From: Torsten Bögershausen @ 2016-11-28 15:54 UTC (permalink / raw)
To: eevee.reply; +Cc: git
In-Reply-To: <777ee899-4cfb-e3b4-da0d-793fde35e412@veekun.com>
On Sun, Nov 27, 2016 at 10:19:35PM -0800, Eevee (Lexy Munroe) wrote:
> I'm working with a repo that used to be all CRLF. At some point it
> was changed to all LF, with `text=auto` in .gitattributes for the
> sake of Windows devs. I'm on Linux and have never touched any
> twiddles relating to line endings. I'm trying to cherry-pick some
> commits from before the switchover.
>
> Straightforward cherry-picking causes entire files at a time to
> conflict, which I've seen before when switching from tabs to spaces.
> So I tried -Xrenormalize and got:
>
> fatal: CRLF would be replaced by LF in [path]
>
Which version of Git are you using, what does
git --version
say?
> The error comes from check_safe_crlf, which warns if checksafe is
> CRLF_SAFE_WARN and dies if it's (presumably) CRLF_SAFE_FAIL. The
> funny thing is that it's CRLF_SAFE_RENORMALIZE.
>
> I don't know what the semantics of this value are, but the caller
> (crlf_to_git) explicitly checks for CRLF_SAFE_RENORMALIZE and
> changes it to CRLF_SAFE_FALSE instead. But that check only happens
> if crlf_action is CRLF_AUTO*, and for me it's CRLF_TEXT_INPUT.
>
> I moved the check to happen regardless of the value of crlf_action,
> and at least in this case, git appears to happily do the right
> thing. So I think this is a bug, but line endings are such a tangle
> that I'm really not sure. :)
>
I am not sure either.
Could you send me the diff you made ?
git diff
I am happy to look into it, (in the amount of time I have).
> The repository in question is ZDoom: https://github.com/rheit/zdoom
> I'm trying to cherry-pick commits from the 3dfloors3 branch (e.g.,
> 9fb2daf58e9d512170859302a1ac0ea9c2ec5993) onto a slightly outdated
> master, 6384e81d0f135a2c292ac3e874f6fe26093f45b1.
This is what I tried:
user@pc:~/NoBackup> cd zdoom/ 9fb2daf58e9d512170859302a1ac0ea9c2ec5993 t9fb2daf5Note: checking out '9fb2daf58e9d512170859302a1ac0ea9c2ec5993'.02a1ac0ea9c2ec5993
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.
If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:
git checkout -b <new-branch-name>
HEAD is now at 9fb2daf... - Force use of GL nodes when 3D floors are present. - Move MAPINFO line modification flags into P_AdjustLine().
user@pc:~/NoBackup/zdoom> git format-patch HEAD^..HEAD --stdout >9fb2daf.patch
user@pc:~/NoBackup/zdoom> git format-patch HEAD^..HEAD --stdout | tr -d "\r" >9fb2daf-noCRLF.patch
user@pc:~/NoBackup/zdoom> git checkout 6384e81d0f135a2c292ac3e874f6fe26093f45b1
Previous HEAD position was 9fb2daf... - Force use of GL nodes when 3D floors are present. - Move MAPINFO line modification flags into P_AdjustLine().
HEAD is now at 6384e81... - Add support for Skulltag ACS IsNetworkGame.
user@pc:~/NoBackup/zdoom> git cherry-pick 9fb2daf58e9d512170859302a1ac0ea9c2ec5993'.02a1ac0ea9c2ec5993
>
user@pc:~/NoBackup/zdoom> git cherry-pick 9fb2daf58e9d512170859302a1ac0ea9c2ec5993
error: could not apply 9fb2daf... - Force use of GL nodes when 3D floors are present.
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
user@pc:~/NoBackup/zdoom> git status
HEAD detached at 6384e81
You are currently cherry-picking commit 9fb2daf.
(fix conflicts and run "git cherry-pick --continue")
(use "git cherry-pick --abort" to cancel the cherry-pick operation)
Unmerged paths:
(use "git add <file>..." to mark resolution)
both modified: src/p_setup.cpp
Untracked files:
(use "git add <file>..." to include in what will be committed)
9fb2daf-noCRLF.patch
9fb2daf.patch
no changes added to commit (use "git add" and/or "git commit -a")
user@pc:~/NoBackup/zdoom> git cherry-pick --abort
user@pc:~/NoBackup/zdoom> git am < 9fb2daf-noCRLF.patch
Applying: - Force use of GL nodes when 3D floors are present. - Move MAPINFO line modification flags into P_AdjustLine().
error: patch failed: src/p_setup.cpp:1798
error: src/p_setup.cpp: patch does not apply
Patch failed at 0001 - Force use of GL nodes when 3D floors are present. - Move MAPINFO line modification flags into P_AdjustLine().
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
The patch did not apply, but for different reasons.
Could you send us, what exactly you did, to help me out ?
^ permalink raw reply
* Re: [PATCH] l10n: de.po: translate 210 new message
From: Jiang Xin @ 2016-11-28 14:21 UTC (permalink / raw)
To: Ralf Thielow
Cc: Git List, Thomas Rast, Jan Krüger, Christian Stimming,
Phillip Sz, matthias.ruester, magnus.goerlitz
In-Reply-To: <20161124182500.6875-1-ralf.thielow@gmail.com>
2016-11-25 2:25 GMT+08:00 Ralf Thielow <ralf.thielow@gmail.com>:
> #: sequencer.c:251
> -#, fuzzy, c-format
> +#, c-format
> msgid "could not write eol to '%s"
Unmatched single quote has been fixed in l10n round 3.
You can rebase and update de.po file.
BTW, Git 2.11.0 will be released tomorrow, please send PR in time.
> -msgstr "Konnte nicht nach '%s' schreiben."
> +msgstr "Konnte EOL nicht nach '%s' schreiben."
--
Jiang Xin
^ permalink raw reply
* Re: [feature request] Make "commit --only" work with new files
From: Johannes Schindelin @ 2016-11-28 14:09 UTC (permalink / raw)
To: Luis Ressel; +Cc: git
In-Reply-To: <20161125175619.19e13e59@gentp.lnet>
Hi Luis,
On Fri, 25 Nov 2016, Luis Ressel wrote:
> currently "git commit --only <file>" only works if <file> is already
> checked into the repo, but not with newly created and still untracked
> files (builtin/commit.c:list_path() throws the error "error: pathspec
> '<file>' did not match any file(s) known to git.")
>
> I don't think this limitation is intented. I've had a look at the
> relevant part of builtin/commit.c, but unfortunately it wasn't obvious
> to me how to fix this.
The best way to go about it is probably to define the desired behavior
first, in the form of a patch to the test suite. Personally, I would
extend t/t7509-commit.sh with a new test case.
Then, once you have that test case (that should be marked with
test_expect_failure, because it fails for now), you can use a debugger
such as gdb to single-step into the relevant function (I guess
"prepare_index()" is a good candidate) to find out why the files are not
added.
From there, it should be easier to figure out what to patch, and how.
Good hunting!
Johannes
^ permalink raw reply
* [PATCH v2 11/11] worktree remove: new command
From: Nguyễn Thái Ngọc Duy @ 2016-11-28 9:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161128094319.16176-1-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-worktree.txt | 21 +++++----
builtin/worktree.c | 79 ++++++++++++++++++++++++++++++++++
contrib/completion/git-completion.bash | 5 ++-
t/t2028-worktree-move.sh | 26 +++++++++++
4 files changed, 121 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 1310513..bbde6b8 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -14,6 +14,7 @@ SYNOPSIS
'git worktree lock' [--reason <string>] <worktree>
'git worktree move' <worktree> <new-path>
'git worktree prune' [-n] [-v] [--expire <expire>]
+'git worktree remove' [--force] <worktree>
'git worktree unlock' <worktree>
DESCRIPTION
@@ -81,6 +82,13 @@ prune::
Prune working tree information in $GIT_DIR/worktrees.
+remove::
+
+Remove a working tree. Only clean working trees (no untracked files
+and no modification in tracked files) can be removed. Unclean working
+trees can be removed with `--force`. The main working tree cannot be
+removed.
+
unlock::
Unlock a working tree, allowing it to be pruned, moved or deleted.
@@ -90,9 +98,10 @@ OPTIONS
-f::
--force::
- By default, `add` refuses to create a new working tree when `<branch>`
- is already checked out by another working tree. This option overrides
- that safeguard.
+ By default, `add` refuses to create a new working tree when
+ `<branch>` is already checked out by another working tree and
+ `remove` refuses to remove an unclean working tree. This option
+ overrides that safeguard.
-b <new-branch>::
-B <new-branch>::
@@ -253,12 +262,6 @@ Multiple checkout in general is still experimental, and the support
for submodules is incomplete. It is NOT recommended to make multiple
checkouts of a superproject.
-git-worktree could provide more automation for tasks currently
-performed manually, such as:
-
-- `remove` to remove a linked working tree and its administrative files (and
- warn if the working tree is dirty)
-
GIT
---
Part of the linkgit:git[1] suite
diff --git a/builtin/worktree.c b/builtin/worktree.c
index e36e4dc..3167409 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -17,6 +17,7 @@ static const char * const worktree_usage[] = {
N_("git worktree lock [<options>] <path>"),
N_("git worktree move <worktree> <new-path>"),
N_("git worktree prune [<options>]"),
+ N_("git worktree remove [<options>] <worktree>"),
N_("git worktree unlock <path>"),
NULL
};
@@ -624,6 +625,82 @@ static int move_worktree(int ac, const char **av, const char *prefix)
return update_worktree_location(wt, dst.buf);
}
+static int remove_worktree(int ac, const char **av, const char *prefix)
+{
+ int force = 0;
+ struct option options[] = {
+ OPT_BOOL(0, "force", &force,
+ N_("force removing even if the worktree is dirty")),
+ OPT_END()
+ };
+ struct worktree **worktrees, *wt;
+ struct strbuf sb = STRBUF_INIT;
+ const char *reason;
+ int ret = 0;
+
+ ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+ if (ac != 1)
+ usage_with_options(worktree_usage, options);
+
+ worktrees = get_worktrees(0);
+ wt = find_worktree(worktrees, prefix, av[0]);
+ if (!wt)
+ die(_("'%s' is not a working directory"), av[0]);
+ if (is_main_worktree(wt))
+ die(_("'%s' is a main working directory"), av[0]);
+ reason = is_worktree_locked(wt);
+ if (reason) {
+ if (*reason)
+ die(_("already locked, reason: %s"), reason);
+ die(_("already locked, no reason"));
+ }
+ if (validate_worktree(wt, 0))
+ return -1;
+
+ if (!force) {
+ struct argv_array child_env = ARGV_ARRAY_INIT;
+ struct child_process cp;
+ char buf[1];
+
+ argv_array_pushf(&child_env, "%s=%s/.git",
+ GIT_DIR_ENVIRONMENT, wt->path);
+ argv_array_pushf(&child_env, "%s=%s",
+ GIT_WORK_TREE_ENVIRONMENT, wt->path);
+ memset(&cp, 0, sizeof(cp));
+ argv_array_pushl(&cp.args, "status", "--porcelain", NULL);
+ cp.env = child_env.argv;
+ cp.git_cmd = 1;
+ cp.dir = wt->path;
+ cp.out = -1;
+ ret = start_command(&cp);
+ if (ret)
+ die_errno(_("failed to run git-status on '%s', code %d"),
+ av[0], ret);
+ ret = xread(cp.out, buf, sizeof(buf));
+ if (ret)
+ die(_("'%s' is dirty, use --force to delete it"), av[0]);
+ close(cp.out);
+ ret = finish_command(&cp);
+ if (ret)
+ die_errno(_("failed to run git-status on '%s', code %d"),
+ av[0], ret);
+ }
+ strbuf_addstr(&sb, wt->path);
+ if (remove_dir_recursively(&sb, 0)) {
+ error_errno(_("failed to delete '%s'"), sb.buf);
+ ret = -1;
+ }
+ strbuf_reset(&sb);
+ strbuf_addstr(&sb, git_common_path("worktrees/%s", wt->id));
+ if (remove_dir_recursively(&sb, 0)) {
+ error_errno(_("failed to delete '%s'"), sb.buf);
+ ret = -1;
+ }
+ strbuf_release(&sb);
+ free_worktrees(worktrees);
+ return ret;
+}
+
int cmd_worktree(int ac, const char **av, const char *prefix)
{
struct option options[] = {
@@ -648,5 +725,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
return unlock_worktree(ac - 1, av + 1, prefix);
if (!strcmp(av[1], "move"))
return move_worktree(ac - 1, av + 1, prefix);
+ if (!strcmp(av[1], "remove"))
+ return remove_worktree(ac - 1, av + 1, prefix);
usage_with_options(worktree_usage, options);
}
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 613e03b..f6855af 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2715,7 +2715,7 @@ _git_whatchanged ()
_git_worktree ()
{
- local subcommands="add list lock move prune unlock"
+ local subcommands="add list lock move prune remove unlock"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__gitcomp "$subcommands"
@@ -2733,6 +2733,9 @@ _git_worktree ()
prune,--*)
__gitcomp "--dry-run --expire --verbose"
;;
+ remove,--*)
+ __gitcomp "--force"
+ ;;
*)
;;
esac
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 74070bd..084acc6 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -89,4 +89,30 @@ test_expect_success 'move main worktree' '
test_must_fail git worktree move . def
'
+test_expect_success 'remove main worktree' '
+ test_must_fail git worktree remove .
+'
+
+test_expect_success 'remove locked worktree' '
+ git worktree lock destination &&
+ test_must_fail git worktree remove destination &&
+ git worktree unlock destination
+'
+
+test_expect_success 'remove worktree with dirty tracked file' '
+ echo dirty >>destination/init.t &&
+ test_must_fail git worktree remove destination
+'
+
+test_expect_success 'remove worktree with untracked file' '
+ git -C destination checkout init.t &&
+ : >destination/untracked &&
+ test_must_fail git worktree remove destination
+'
+
+test_expect_success 'force remove worktree with untracked file' '
+ git worktree remove --force destination &&
+ test_path_is_missing destination
+'
+
test_done
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 10/11] worktree move: refuse to move worktrees with submodules
From: Nguyễn Thái Ngọc Duy @ 2016-11-28 9:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161128094319.16176-1-pclouds@gmail.com>
Submodules contains .git files with relative paths. After a worktree
move, these files need to be updated or they may point to nowhere.
This is a bandage patch to make sure "worktree move" don't break
people's worktrees by accident. When .git file update code is in
place, this validate_no_submodules() could be removed.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/worktree.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index f732a74..e36e4dc 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -525,6 +525,27 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
return ret;
}
+static void validate_no_submodules(const struct worktree *wt)
+{
+ struct index_state istate = {0};
+ int i, found_submodules = 0;
+
+ if (read_index_from(&istate, worktree_git_path(wt, "index")) > 0) {
+ for (i = 0; i < istate.cache_nr; i++) {
+ struct cache_entry *ce = istate.cache[i];
+
+ if (S_ISGITLINK(ce->ce_mode)) {
+ found_submodules = 1;
+ break;
+ }
+ }
+ }
+ discard_index(&istate);
+
+ if (found_submodules)
+ die(_("This working tree contains submodules and cannot be moved yet"));
+}
+
static int move_worktree(int ac, const char **av, const char *prefix)
{
struct option options[] = {
@@ -565,6 +586,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)
if (validate_worktree(wt, 0))
return -1;
+ validate_no_submodules(wt);
+
if (is_directory(dst.buf)) {
const char *sep = find_last_dir_sep(wt->path);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 09/11] worktree move: accept destination as directory
From: Nguyễn Thái Ngọc Duy @ 2016-11-28 9:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161128094319.16176-1-pclouds@gmail.com>
Similar to "mv a b/", which is actually "mv a b/a", we extract basename
of source worktree and create a directory of the same name at
destination if dst path is a directory.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/worktree.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index f114965..f732a74 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -541,7 +541,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)
strbuf_addstr(&dst, prefix_filename(prefix,
strlen(prefix),
av[1]));
- if (file_exists(dst.buf))
+ if (is_directory(dst.buf))
+ /*
+ * keep going, dst will be appended after we get the
+ * source's absolute path
+ */
+ ;
+ else if (file_exists(dst.buf))
die(_("target '%s' already exists"), av[1]);
worktrees = get_worktrees(0);
@@ -559,6 +565,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)
if (validate_worktree(wt, 0))
return -1;
+ if (is_directory(dst.buf)) {
+ const char *sep = find_last_dir_sep(wt->path);
+
+ if (!sep)
+ die(_("could not figure out destination name from '%s'"),
+ wt->path);
+ strbuf_addstr(&dst, sep);
+ if (file_exists(dst.buf))
+ die(_("target '%s' already exists"), dst.buf);
+ }
+
/*
* First try. Atomically move, and probably cheaper, if both
* source and target are on the same file system.
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 08/11] worktree move: new command
From: Nguyễn Thái Ngọc Duy @ 2016-11-28 9:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161128094319.16176-1-pclouds@gmail.com>
There are two options to move the main worktree, but both have
complications, so it's not implemented yet. Anyway the options are:
- convert the main worktree to a linked one and move it away, leave the
git repository where it is. The repo essentially becomes bare after
this move.
- move the repository with the main worktree. The tricky part is make
sure all file descriptors to the repository are closed, or it may
fail on Windows.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-worktree.txt | 7 +++-
builtin/worktree.c | 62 ++++++++++++++++++++++++++++++++++
contrib/completion/git-completion.bash | 2 +-
t/t2028-worktree-move.sh | 30 ++++++++++++++++
4 files changed, 99 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index e257c19..1310513 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -12,6 +12,7 @@ SYNOPSIS
'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
'git worktree list' [--porcelain]
'git worktree lock' [--reason <string>] <worktree>
+'git worktree move' <worktree> <new-path>
'git worktree prune' [-n] [-v] [--expire <expire>]
'git worktree unlock' <worktree>
@@ -71,6 +72,11 @@ files from being pruned automatically. This also prevents it from
being moved or deleted. Optionally, specify a reason for the lock
with `--reason`.
+move::
+
+Move a working tree to a new location. Note that the main working tree
+cannot be moved yet.
+
prune::
Prune working tree information in $GIT_DIR/worktrees.
@@ -252,7 +258,6 @@ performed manually, such as:
- `remove` to remove a linked working tree and its administrative files (and
warn if the working tree is dirty)
-- `mv` to move or rename a working tree and update its administrative files
GIT
---
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9a97e37..f114965 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -15,6 +15,7 @@ static const char * const worktree_usage[] = {
N_("git worktree add [<options>] <path> [<branch>]"),
N_("git worktree list [<options>]"),
N_("git worktree lock [<options>] <path>"),
+ N_("git worktree move <worktree> <new-path>"),
N_("git worktree prune [<options>]"),
N_("git worktree unlock <path>"),
NULL
@@ -524,6 +525,65 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
return ret;
}
+static int move_worktree(int ac, const char **av, const char *prefix)
+{
+ struct option options[] = {
+ OPT_END()
+ };
+ struct worktree **worktrees, *wt;
+ struct strbuf dst = STRBUF_INIT;
+ const char *reason;
+
+ ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+ if (ac != 2)
+ usage_with_options(worktree_usage, options);
+
+ strbuf_addstr(&dst, prefix_filename(prefix,
+ strlen(prefix),
+ av[1]));
+ if (file_exists(dst.buf))
+ die(_("target '%s' already exists"), av[1]);
+
+ worktrees = get_worktrees(0);
+ wt = find_worktree(worktrees, prefix, av[0]);
+ if (!wt)
+ die(_("'%s' is not a working directory"), av[0]);
+ if (is_main_worktree(wt))
+ die(_("'%s' is a main working directory"), av[0]);
+ reason = is_worktree_locked(wt);
+ if (reason) {
+ if (*reason)
+ die(_("already locked, reason: %s"), reason);
+ die(_("already locked, no reason"));
+ }
+ if (validate_worktree(wt, 0))
+ return -1;
+
+ /*
+ * First try. Atomically move, and probably cheaper, if both
+ * source and target are on the same file system.
+ */
+ if (rename(wt->path, dst.buf) == -1) {
+ if (errno != EXDEV)
+ die_errno(_("failed to move '%s' to '%s'"),
+ wt->path, dst.buf);
+
+ /* second try.. */
+ if (copy_dir_recursively(wt->path, dst.buf))
+ die(_("failed to copy '%s' to '%s'"),
+ wt->path, dst.buf);
+ else {
+ struct strbuf sb = STRBUF_INIT;
+
+ strbuf_addstr(&sb, wt->path);
+ (void)remove_dir_recursively(&sb, 0);
+ strbuf_release(&sb);
+ }
+ }
+
+ return update_worktree_location(wt, dst.buf);
+}
+
int cmd_worktree(int ac, const char **av, const char *prefix)
{
struct option options[] = {
@@ -546,5 +606,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
return lock_worktree(ac - 1, av + 1, prefix);
if (!strcmp(av[1], "unlock"))
return unlock_worktree(ac - 1, av + 1, prefix);
+ if (!strcmp(av[1], "move"))
+ return move_worktree(ac - 1, av + 1, prefix);
usage_with_options(worktree_usage, options);
}
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 21016bf..613e03b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2715,7 +2715,7 @@ _git_whatchanged ()
_git_worktree ()
{
- local subcommands="add list lock prune unlock"
+ local subcommands="add list lock move prune unlock"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__gitcomp "$subcommands"
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 8298aaf..74070bd 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -59,4 +59,34 @@ test_expect_success 'unlock worktree twice' '
test_path_is_missing .git/worktrees/source/locked
'
+test_expect_success 'move non-worktree' '
+ mkdir abc &&
+ test_must_fail git worktree move abc def
+'
+
+test_expect_success 'move locked worktree' '
+ git worktree lock source &&
+ test_must_fail git worktree move source destination &&
+ git worktree unlock source
+'
+
+test_expect_success 'move worktree' '
+ git worktree move source destination &&
+ test_path_is_missing source &&
+ git worktree list --porcelain | grep "^worktree" >actual &&
+ cat <<-EOF >expected &&
+ worktree $TRASH_DIRECTORY
+ worktree $TRASH_DIRECTORY/destination
+ worktree $TRASH_DIRECTORY/elsewhere
+ EOF
+ test_cmp expected actual &&
+ git -C destination log --format=%s >actual2 &&
+ echo init >expected2 &&
+ test_cmp expected2 actual2
+'
+
+test_expect_success 'move main worktree' '
+ test_must_fail git worktree move . def
+'
+
test_done
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 07/11] worktree.c: add update_worktree_location()
From: Nguyễn Thái Ngọc Duy @ 2016-11-28 9:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161128094319.16176-1-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
worktree.c | 21 +++++++++++++++++++++
worktree.h | 6 ++++++
2 files changed, 27 insertions(+)
diff --git a/worktree.c b/worktree.c
index 929072a..7684951 100644
--- a/worktree.c
+++ b/worktree.c
@@ -354,6 +354,27 @@ int validate_worktree(const struct worktree *wt, int quiet)
return 0;
}
+int update_worktree_location(struct worktree *wt, const char *path_)
+{
+ struct strbuf path = STRBUF_INIT;
+ int ret = 0;
+
+ if (is_main_worktree(wt))
+ return 0;
+
+ strbuf_add_absolute_path(&path, path_);
+ if (fspathcmp(wt->path, path.buf)) {
+ write_file(git_common_path("worktrees/%s/gitdir",
+ wt->id),
+ "%s/.git", real_path(path.buf));
+ free(wt->path);
+ wt->path = strbuf_detach(&path, NULL);
+ ret = 0;
+ }
+ strbuf_release(&path);
+ return ret;
+}
+
int is_worktree_being_rebased(const struct worktree *wt,
const char *target)
{
diff --git a/worktree.h b/worktree.h
index 4433db2..1ee03f4 100644
--- a/worktree.h
+++ b/worktree.h
@@ -57,6 +57,12 @@ extern const char *is_worktree_locked(struct worktree *wt);
*/
extern int validate_worktree(const struct worktree *wt, int quiet);
+/*
+ * Update worktrees/xxx/gitdir with the new path.
+ */
+extern int update_worktree_location(struct worktree *wt,
+ const char *path_);
+
/*
* Free up the memory for worktree(s)
*/
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 06/11] worktree.c: add validate_worktree()
From: Nguyễn Thái Ngọc Duy @ 2016-11-28 9:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161128094319.16176-1-pclouds@gmail.com>
This function is later used by "worktree move" and "worktree remove"
to ensure that we have a good connection between the repository and
the worktree. For example, if a worktree is moved manually, the
worktree location recorded in $GIT_DIR/worktrees/.../gitdir is
incorrect and we should not move that one.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
worktree.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
worktree.h | 5 +++++
2 files changed, 68 insertions(+)
diff --git a/worktree.c b/worktree.c
index eb61212..929072a 100644
--- a/worktree.c
+++ b/worktree.c
@@ -291,6 +291,69 @@ const char *is_worktree_locked(struct worktree *wt)
return wt->lock_reason;
}
+static int report(int quiet, const char *fmt, ...)
+{
+ va_list params;
+
+ if (quiet)
+ return -1;
+
+ va_start(params, fmt);
+ vfprintf(stderr, fmt, params);
+ fputc('\n', stderr);
+ va_end(params);
+ return -1;
+}
+
+int validate_worktree(const struct worktree *wt, int quiet)
+{
+ struct strbuf sb = STRBUF_INIT;
+ const char *path;
+ int err;
+
+ if (is_main_worktree(wt)) {
+ /*
+ * Main worktree using .git file to point to the
+ * repository would make it impossible to know where
+ * the actual worktree is if this function is executed
+ * from another worktree. No .git file support for now.
+ */
+ strbuf_addf(&sb, "%s/.git", wt->path);
+ if (!is_directory(sb.buf)) {
+ strbuf_release(&sb);
+ return report(quiet, _("'%s/.git' at main worktree is not the repository directory"),
+ wt->path);
+ }
+ return 0;
+ }
+
+ /*
+ * Make sure "gitdir" file points to a real .git file and that
+ * file points back here.
+ */
+ if (!is_absolute_path(wt->path))
+ return report(quiet, _("'%s' file does not contain absolute path to the worktree location"),
+ git_common_path("worktrees/%s/gitdir", wt->id));
+
+ strbuf_addf(&sb, "%s/.git", wt->path);
+ if (!file_exists(sb.buf)) {
+ strbuf_release(&sb);
+ return report(quiet, _("'%s/.git' does not exist"), wt->path);
+ }
+
+ path = read_gitfile_gently(sb.buf, &err);
+ strbuf_release(&sb);
+ if (!path)
+ return report(quiet, _("'%s/.git' is not a .git file, error code %d"),
+ wt->path, err);
+
+ if (fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id))))
+ return report(quiet, _("'%s' does not point back to"),
+ wt->path, git_common_path("worktrees/%s", wt->id));
+
+ return 0;
+}
+
int is_worktree_being_rebased(const struct worktree *wt,
const char *target)
{
diff --git a/worktree.h b/worktree.h
index d59ce1f..4433db2 100644
--- a/worktree.h
+++ b/worktree.h
@@ -52,6 +52,11 @@ extern int is_main_worktree(const struct worktree *wt);
*/
extern const char *is_worktree_locked(struct worktree *wt);
+/*
+ * Return zero if the worktree is in good condition.
+ */
+extern int validate_worktree(const struct worktree *wt, int quiet);
+
/*
* Free up the memory for worktree(s)
*/
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 05/11] copy.c: convert copy_file() to copy_dir_recursively()
From: Nguyễn Thái Ngọc Duy @ 2016-11-28 9:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161128094319.16176-1-pclouds@gmail.com>
This finally enables busybox's copy_file() code under a new name
(because "copy_file" is already taken in Git code base). Because this
comes from busybox, POSIXy (or even Linuxy) behavior is expected. More
changes may be needed for Windows support.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
cache.h | 1 +
copy.c | 216 ++++++++++++++++++++++++++++++++++++++++++++++++++++------------
2 files changed, 179 insertions(+), 38 deletions(-)
diff --git a/cache.h b/cache.h
index a50a61a..a9a72f8 100644
--- a/cache.h
+++ b/cache.h
@@ -1857,6 +1857,7 @@ extern void fprintf_or_die(FILE *, const char *fmt, ...);
extern int copy_fd(int ifd, int ofd);
extern int copy_file(const char *dst, const char *src, int mode);
extern int copy_file_with_time(const char *dst, const char *src, int mode);
+extern int copy_dir_recursively(const char *source, const char *dest);
extern void write_or_die(int fd, const void *buf, size_t count);
extern void fsync_or_die(int fd, const char *);
diff --git a/copy.c b/copy.c
index 60c7d8a..8ef4b23 100644
--- a/copy.c
+++ b/copy.c
@@ -1,4 +1,6 @@
#include "cache.h"
+#include "dir.h"
+#include "hashmap.h"
int copy_fd(int ifd, int ofd)
{
@@ -66,21 +68,126 @@ int copy_file_with_time(const char *dst, const char *src, int mode)
return status;
}
-#if 0
-/* Return:
- * -1 error, copy not made
- * 0 copy is made or user answered "no" in interactive mode
- * (failures to preserve mode/owner/times are not reported in exit code)
+struct inode_key {
+ struct hashmap_entry entry;
+ ino_t ino;
+ dev_t dev;
+ /*
+ * Reportedly, on cramfs a file and a dir can have same ino.
+ * Need to also remember "file/dir" bit:
+ */
+ char isdir; /* bool */
+};
+
+struct inode_value {
+ struct inode_key key;
+ char name[FLEX_ARRAY];
+};
+
+#define HASH_SIZE 311u /* Should be prime */
+static inline unsigned hash_inode(ino_t i)
+{
+ return i % HASH_SIZE;
+}
+
+static int inode_cmp(const void *entry, const void *entry_or_key,
+ const void *keydata)
+{
+ const struct inode_value *inode = entry;
+ const struct inode_key *key = entry_or_key;
+
+ return !(inode->key.ino == key->ino &&
+ inode->key.dev == key->dev &&
+ inode->key.isdir == key->isdir);
+}
+
+static const char *is_in_ino_dev_hashtable(const struct hashmap *map,
+ const struct stat *st)
+{
+ struct inode_key key;
+ struct inode_value *value;
+
+ key.entry.hash = hash_inode(st->st_ino);
+ key.ino = st->st_ino;
+ key.dev = st->st_dev;
+ key.isdir = !!S_ISDIR(st->st_mode);
+ value = hashmap_get(map, &key, NULL);
+ return value ? value->name : NULL;
+}
+
+static void add_to_ino_dev_hashtable(struct hashmap *map,
+ const struct stat *st,
+ const char *path)
+{
+ struct inode_value *v;
+ int len = strlen(path);
+
+ v = xmalloc(offsetof(struct inode_value, name) + len + 1);
+ v->key.entry.hash = hash_inode(st->st_ino);
+ v->key.ino = st->st_ino;
+ v->key.dev = st->st_dev;
+ v->key.isdir = !!S_ISDIR(st->st_mode);
+ memcpy(v->name, path, len + 1);
+ hashmap_add(map, v);
+}
+
+/*
+ * Find out if the last character of a string matches the one given.
+ * Don't underrun the buffer if the string length is 0.
*/
-int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
+static inline char *last_char_is(const char *s, int c)
+{
+ if (s && *s) {
+ size_t sz = strlen(s) - 1;
+ s += sz;
+ if ((unsigned char)*s == c)
+ return (char *)s;
+ }
+ return NULL;
+}
+
+static inline char *concat_path_file(const char *path, const char *filename)
+{
+ struct strbuf sb = STRBUF_INIT;
+ char *lc;
+
+ if (!path)
+ path = "";
+ lc = last_char_is(path, '/');
+ while (*filename == '/')
+ filename++;
+ strbuf_addf(&sb, "%s%s%s", path, !lc ? "/" : "", filename);
+ return strbuf_detach(&sb, NULL);
+}
+
+static char *concat_subpath_file(const char *path, const char *f)
+{
+ if (f && is_dot_or_dotdot(f))
+ return NULL;
+ return concat_path_file(path, f);
+}
+
+static int do_unlink(const char *dest)
+{
+ int e = errno;
+
+ if (unlink(dest) < 0) {
+ errno = e; /* do not use errno from unlink */
+ return error_errno(_("can't create '%s'"), dest);
+ }
+ return 0;
+}
+
+static int copy_dir_1(struct hashmap *inode_map,
+ const char *source,
+ const char *dest)
{
/* This is a recursive function, try to minimize stack usage */
- /* NB: each struct stat is ~100 bytes */
struct stat source_stat;
struct stat dest_stat;
- smallint retval = 0;
- smallint dest_exists = 0;
- smallint ovr;
+ int retval = 0;
+ int dest_exists = 0;
+ int ovr;
if (lstat(source, &source_stat) < 0)
return error_errno(_("can't stat '%s'"), source);
@@ -102,7 +209,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
mode_t saved_umask = 0;
/* Did we ever create source ourself before? */
- tp = is_in_ino_dev_hashtable(&source_stat);
+ tp = is_in_ino_dev_hashtable(inode_map, &source_stat);
if (tp)
/* We did! it's a recursion! man the lifeboats... */
return error(_("recursion detected, omitting directory '%s'"),
@@ -132,11 +239,12 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
if (lstat(dest, &dest_stat) < 0)
return error_errno(_("can't stat '%s'"), dest);
}
+
/*
* remember (dev,inode) of each created dir. name is
* not remembered
*/
- add_to_ino_dev_hashtable(&dest_stat, NULL);
+ add_to_ino_dev_hashtable(inode_map, &dest_stat, "");
/* Recursively copy files in SOURCE */
dp = opendir(source);
@@ -152,7 +260,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
if (!new_source)
continue;
new_dest = concat_path_file(dest, d->d_name);
- if (copy_file(new_source, new_dest, flags & ~FILEUTILS_DEREFERENCE_L0) < 0)
+ if (copy_dir_1(inode_map, new_source, new_dest) < 0)
retval = -1;
free(new_source);
free(new_dest);
@@ -177,53 +285,57 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
goto dont_cat;
}
- if (ENABLE_FEATURE_PRESERVE_HARDLINKS) {
+ if (1 /*ENABLE_FEATURE_PRESERVE_HARDLINKS*/) {
const char *link_target;
- link_target = is_in_ino_dev_hashtable(&source_stat);
+ link_target = is_in_ino_dev_hashtable(inode_map, &source_stat);
if (link_target) {
if (link(link_target, dest) < 0) {
- ovr = ask_and_unlink(dest, flags);
- if (ovr <= 0)
+ ovr = do_unlink(dest);
+ if (ovr < 0)
return ovr;
if (link(link_target, dest) < 0)
return error_errno(_("can't create link '%s'"), dest);
}
return 0;
}
- add_to_ino_dev_hashtable(&source_stat, dest);
+ add_to_ino_dev_hashtable(inode_map, &source_stat, dest);
}
- src_fd = open_or_warn(source, O_RDONLY);
+ src_fd = open(source, O_RDONLY);
if (src_fd < 0)
- return -1;
+ return error_errno(_("can't open '%s'"), source);
/* Do not try to open with weird mode fields */
new_mode = source_stat.st_mode;
if (!S_ISREG(source_stat.st_mode))
new_mode = 0666;
- /* POSIX way is a security problem versus (sym)link attacks */
- if (!ENABLE_FEATURE_NON_POSIX_CP) {
- dst_fd = open(dest, O_WRONLY|O_CREAT|O_TRUNC, new_mode);
- } else { /* safe way: */
- dst_fd = open(dest, O_WRONLY|O_CREAT|O_EXCL, new_mode);
- }
+ dst_fd = open(dest, O_WRONLY|O_CREAT|O_EXCL, new_mode);
if (dst_fd == -1) {
- ovr = ask_and_unlink(dest, flags);
- if (ovr <= 0) {
+ ovr = do_unlink(dest);
+ if (ovr < 0) {
close(src_fd);
return ovr;
}
/* It shouldn't exist. If it exists, do not open (symlink attack?) */
- dst_fd = open3_or_warn(dest, O_WRONLY|O_CREAT|O_EXCL, new_mode);
+ dst_fd = open(dest, O_WRONLY|O_CREAT|O_EXCL, new_mode);
if (dst_fd < 0) {
close(src_fd);
- return -1;
+ return error_errno(_("can't open '%s'"), dest);
}
}
- if (bb_copyfd_eof(src_fd, dst_fd) == -1)
+ switch (copy_fd(src_fd, dst_fd)) {
+ case COPY_READ_ERROR:
+ error(_("copy-fd: read returned %s"), strerror(errno));
retval = -1;
+ break;
+ case COPY_WRITE_ERROR:
+ error(_("copy-fd: write returned %s"), strerror(errno));
+ retval = -1;
+ break;
+ }
+
/* Careful with writing... */
if (close(dst_fd) < 0)
retval = error_errno(_("error writing to '%s'"), dest);
@@ -243,19 +355,28 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
/* We are lazy here, a bit lax with races... */
if (dest_exists) {
errno = EEXIST;
- ovr = ask_and_unlink(dest, flags);
- if (ovr <= 0)
+ ovr = do_unlink(dest);
+ if (ovr < 0)
return ovr;
}
if (S_ISLNK(source_stat.st_mode)) {
- char *lpath = xmalloc_readlink_or_warn(source);
- if (lpath) {
- int r = symlink(lpath, dest);
- free(lpath);
+ struct strbuf lpath = STRBUF_INIT;
+ if (!strbuf_readlink(&lpath, source, 0)) {
+ int r = symlink(lpath.buf, dest);
+ strbuf_release(&lpath);
if (r < 0)
return error_errno(_("can't create symlink '%s'"), dest);
if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
error_errno(_("can't preserve %s of '%s'"), "ownership", dest);
+ } else {
+ /* EINVAL => "file: Invalid argument" => puzzled user */
+ const char *errmsg = _("not a symlink");
+ int err = errno;
+
+ if (err != EINVAL)
+ errmsg = strerror(err);
+ error(_("%s: cannot read link: %s"), source, errmsg);
+ strbuf_release(&lpath);
}
/*
* _Not_ jumping to preserve_mode_ugid_time: symlinks
@@ -293,4 +414,23 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
return retval;
}
-#endif
+
+/*
+ * Return:
+ * -1 error, copy not made
+ * 0 copy is made
+ *
+ * Failures to preserve mode/owner/times are not reported in exit
+ * code. No support for preserving SELinux security context. Symlinks
+ * and hardlinks are preserved.
+ */
+int copy_dir_recursively(const char *source, const char *dest)
+{
+ int ret;
+ struct hashmap inode_map;
+
+ hashmap_init(&inode_map, inode_cmp, 1024);
+ ret = copy_dir_1(&inode_map, source, dest);
+ hashmap_free(&inode_map, 1);
+ return ret;
+}
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 04/11] copy.c: style fix
From: Nguyễn Thái Ngọc Duy @ 2016-11-28 9:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161128094319.16176-1-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
copy.c | 50 +++++++++++++++++++++++++++++---------------------
1 file changed, 29 insertions(+), 21 deletions(-)
diff --git a/copy.c b/copy.c
index 074b609..60c7d8a 100644
--- a/copy.c
+++ b/copy.c
@@ -111,8 +111,10 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
if (dest_exists) {
if (!S_ISDIR(dest_stat.st_mode))
return error(_("target '%s' is not a directory"), dest);
- /* race here: user can substitute a symlink between
- * this check and actual creation of files inside dest */
+ /*
+ * race here: user can substitute a symlink between
+ * this check and actual creation of files inside dest
+ */
} else {
/* Create DEST */
mode_t mode;
@@ -130,22 +132,24 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
if (lstat(dest, &dest_stat) < 0)
return error_errno(_("can't stat '%s'"), dest);
}
- /* remember (dev,inode) of each created dir.
- * NULL: name is not remembered */
+ /*
+ * remember (dev,inode) of each created dir. name is
+ * not remembered
+ */
add_to_ino_dev_hashtable(&dest_stat, NULL);
/* Recursively copy files in SOURCE */
dp = opendir(source);
- if (dp == NULL) {
+ if (!dp) {
retval = -1;
goto preserve_mode_ugid_time;
}
- while ((d = readdir(dp)) != NULL) {
+ while ((d = readdir(dp))) {
char *new_source, *new_dest;
new_source = concat_subpath_file(source, d->d_name);
- if (new_source == NULL)
+ if (!new_source)
continue;
new_dest = concat_path_file(dest, d->d_name);
if (copy_file(new_source, new_dest, flags & ~FILEUTILS_DEREFERENCE_L0) < 0)
@@ -155,16 +159,15 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
}
closedir(dp);
- if (!dest_exists
- && chmod(dest, source_stat.st_mode & ~saved_umask) < 0
- ) {
+ if (!dest_exists &&
+ chmod(dest, source_stat.st_mode & ~saved_umask) < 0) {
error_errno(_("can't preserve permissions of '%s'"), dest);
/* retval = -1; - WRONG! copy *WAS* made */
}
goto preserve_mode_ugid_time;
}
- if (S_ISREG(source_stat.st_mode) ) { /* "cp [-opts] regular_file thing2" */
+ if (S_ISREG(source_stat.st_mode)) { /* "cp [-opts] regular_file thing2" */
int src_fd;
int dst_fd;
mode_t new_mode;
@@ -199,7 +202,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
if (!S_ISREG(source_stat.st_mode))
new_mode = 0666;
- // POSIX way is a security problem versus (sym)link attacks
+ /* POSIX way is a security problem versus (sym)link attacks */
if (!ENABLE_FEATURE_NON_POSIX_CP) {
dst_fd = open(dest, O_WRONLY|O_CREAT|O_TRUNC, new_mode);
} else { /* safe way: */
@@ -226,13 +229,15 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
retval = error_errno(_("error writing to '%s'"), dest);
/* ...but read size is already checked by bb_copyfd_eof */
close(src_fd);
- /* "cp /dev/something new_file" should not
- * copy mode of /dev/something */
+ /*
+ * "cp /dev/something new_file" should not
+ * copy mode of /dev/something
+ */
if (!S_ISREG(source_stat.st_mode))
return retval;
goto preserve_mode_ugid_time;
}
- dont_cat:
+dont_cat:
/* Source is a symlink or a special file */
/* We are lazy here, a bit lax with races... */
@@ -252,20 +257,23 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
error_errno(_("can't preserve %s of '%s'"), "ownership", dest);
}
- /* _Not_ jumping to preserve_mode_ugid_time:
- * symlinks don't have those */
+ /*
+ * _Not_ jumping to preserve_mode_ugid_time: symlinks
+ * don't have those
+ */
return 0;
}
- if (S_ISBLK(source_stat.st_mode) || S_ISCHR(source_stat.st_mode)
- || S_ISSOCK(source_stat.st_mode) || S_ISFIFO(source_stat.st_mode)
- ) {
+ if (S_ISBLK(source_stat.st_mode) ||
+ S_ISCHR(source_stat.st_mode) ||
+ S_ISSOCK(source_stat.st_mode) ||
+ S_ISFIFO(source_stat.st_mode)) {
if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0)
return error_errno(_("can't create '%s'"), dest);
} else
return error(_("unrecognized file '%s' with mode %x"),
source, source_stat.st_mode);
- preserve_mode_ugid_time:
+preserve_mode_ugid_time:
if (1 /*FILEUTILS_PRESERVE_STATUS*/) {
struct timeval times[2];
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 03/11] copy.c: convert bb_(p)error_msg to error(_errno)
From: Nguyễn Thái Ngọc Duy @ 2016-11-28 9:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161128094319.16176-1-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
copy.c | 85 ++++++++++++++++++++++++------------------------------------------
1 file changed, 31 insertions(+), 54 deletions(-)
diff --git a/copy.c b/copy.c
index b7a87f1..074b609 100644
--- a/copy.c
+++ b/copy.c
@@ -82,23 +82,16 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
smallint dest_exists = 0;
smallint ovr;
- if (lstat(source, &source_stat) < 0) {
- bb_perror_msg("can't stat '%s'", source);
- return -1;
- }
+ if (lstat(source, &source_stat) < 0)
+ return error_errno(_("can't stat '%s'"), source);
if (lstat(dest, &dest_stat) < 0) {
- if (errno != ENOENT) {
- bb_perror_msg("can't stat '%s'", dest);
- return -1;
- }
+ if (errno != ENOENT)
+ return error_errno(_("can't stat '%s'"), dest);
} else {
- if (source_stat.st_dev == dest_stat.st_dev
- && source_stat.st_ino == dest_stat.st_ino
- ) {
- bb_error_msg("'%s' and '%s' are the same file", source, dest);
- return -1;
- }
+ if (source_stat.st_dev == dest_stat.st_dev &&
+ source_stat.st_ino == dest_stat.st_ino)
+ return error(_("'%s' and '%s' are the same file"), source, dest);
dest_exists = 1;
}
@@ -110,18 +103,14 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
/* Did we ever create source ourself before? */
tp = is_in_ino_dev_hashtable(&source_stat);
- if (tp) {
+ if (tp)
/* We did! it's a recursion! man the lifeboats... */
- bb_error_msg("recursion detected, omitting directory '%s'",
- source);
- return -1;
- }
+ return error(_("recursion detected, omitting directory '%s'"),
+ source);
if (dest_exists) {
- if (!S_ISDIR(dest_stat.st_mode)) {
- bb_error_msg("target '%s' is not a directory", dest);
- return -1;
- }
+ if (!S_ISDIR(dest_stat.st_mode))
+ return error(_("target '%s' is not a directory"), dest);
/* race here: user can substitute a symlink between
* this check and actual creation of files inside dest */
} else {
@@ -134,15 +123,12 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
mode |= S_IRWXU;
if (mkdir(dest, mode) < 0) {
umask(saved_umask);
- bb_perror_msg("can't create directory '%s'", dest);
- return -1;
+ return error_errno(_("can't create directory '%s'"), dest);
}
umask(saved_umask);
/* need stat info for add_to_ino_dev_hashtable */
- if (lstat(dest, &dest_stat) < 0) {
- bb_perror_msg("can't stat '%s'", dest);
- return -1;
- }
+ if (lstat(dest, &dest_stat) < 0)
+ return error_errno(_("can't stat '%s'"), dest);
}
/* remember (dev,inode) of each created dir.
* NULL: name is not remembered */
@@ -172,7 +158,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
if (!dest_exists
&& chmod(dest, source_stat.st_mode & ~saved_umask) < 0
) {
- bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
+ error_errno(_("can't preserve permissions of '%s'"), dest);
/* retval = -1; - WRONG! copy *WAS* made */
}
goto preserve_mode_ugid_time;
@@ -196,10 +182,8 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
ovr = ask_and_unlink(dest, flags);
if (ovr <= 0)
return ovr;
- if (link(link_target, dest) < 0) {
- bb_perror_msg("can't create link '%s'", dest);
- return -1;
- }
+ if (link(link_target, dest) < 0)
+ return error_errno(_("can't create link '%s'"), dest);
}
return 0;
}
@@ -238,10 +222,8 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
if (bb_copyfd_eof(src_fd, dst_fd) == -1)
retval = -1;
/* Careful with writing... */
- if (close(dst_fd) < 0) {
- bb_perror_msg("error writing to '%s'", dest);
- retval = -1;
- }
+ if (close(dst_fd) < 0)
+ retval = error_errno(_("error writing to '%s'"), dest);
/* ...but read size is already checked by bb_copyfd_eof */
close(src_fd);
/* "cp /dev/something new_file" should not
@@ -265,12 +247,10 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
if (lpath) {
int r = symlink(lpath, dest);
free(lpath);
- if (r < 0) {
- bb_perror_msg("can't create symlink '%s'", dest);
- return -1;
- }
+ if (r < 0)
+ return error_errno(_("can't create symlink '%s'"), dest);
if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
- bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+ error_errno(_("can't preserve %s of '%s'"), "ownership", dest);
}
/* _Not_ jumping to preserve_mode_ugid_time:
* symlinks don't have those */
@@ -279,14 +259,11 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
if (S_ISBLK(source_stat.st_mode) || S_ISCHR(source_stat.st_mode)
|| S_ISSOCK(source_stat.st_mode) || S_ISFIFO(source_stat.st_mode)
) {
- if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0) {
- bb_perror_msg("can't create '%s'", dest);
- return -1;
- }
- } else {
- bb_error_msg("unrecognized file '%s' with mode %x", source, source_stat.st_mode);
- return -1;
- }
+ if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0)
+ return error_errno(_("can't create '%s'"), dest);
+ } else
+ return error(_("unrecognized file '%s' with mode %x"),
+ source, source_stat.st_mode);
preserve_mode_ugid_time:
@@ -297,13 +274,13 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
times[1].tv_usec = times[0].tv_usec = 0;
/* BTW, utimes sets usec-precision time - just FYI */
if (utimes(dest, times) < 0)
- bb_perror_msg("can't preserve %s of '%s'", "times", dest);
+ error_errno(_("can't preserve %s of '%s'"), "times", dest);
if (chown(dest, source_stat.st_uid, source_stat.st_gid) < 0) {
source_stat.st_mode &= ~(S_ISUID | S_ISGID);
- bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+ error_errno(_("can't preserve %s of '%s'"), "ownership", dest);
}
if (chmod(dest, source_stat.st_mode) < 0)
- bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
+ error_errno(_("can't preserve %s of '%s'"), "permissions", dest);
}
return retval;
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 02/11] copy.c: delete unused code in copy_file()
From: Nguyễn Thái Ngọc Duy @ 2016-11-28 9:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161128094319.16176-1-pclouds@gmail.com>
- selinux preservation code
- make-link code
- delete link dereference code
- non-recursive copy code
- stat no preservation code
- verbose printing code
Some of these are "cp" features that we don't need (for "git worktree
move"). Some do not make sense in source-control context (SELinux).
Some probably need a reimplementation to better fit in (verbose
printing code).
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
copy.c | 101 +++++------------------------------------------------------------
1 file changed, 7 insertions(+), 94 deletions(-)
diff --git a/copy.c b/copy.c
index 79623ac..b7a87f1 100644
--- a/copy.c
+++ b/copy.c
@@ -82,14 +82,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
smallint dest_exists = 0;
smallint ovr;
-/* Inverse of cp -d ("cp without -d") */
-#define FLAGS_DEREF (flags & (FILEUTILS_DEREFERENCE + FILEUTILS_DEREFERENCE_L0))
-
- if ((FLAGS_DEREF ? stat : lstat)(source, &source_stat) < 0) {
- /* This may be a dangling symlink.
- * Making [sym]links to dangling symlinks works, so... */
- if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK))
- goto make_links;
+ if (lstat(source, &source_stat) < 0) {
bb_perror_msg("can't stat '%s'", source);
return -1;
}
@@ -109,35 +102,12 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
dest_exists = 1;
}
-#if ENABLE_SELINUX
- if ((flags & FILEUTILS_PRESERVE_SECURITY_CONTEXT) && is_selinux_enabled() > 0) {
- security_context_t con;
- if (lgetfilecon(source, &con) >= 0) {
- if (setfscreatecon(con) < 0) {
- bb_perror_msg("can't set setfscreatecon %s", con);
- freecon(con);
- return -1;
- }
- } else if (errno == ENOTSUP || errno == ENODATA) {
- setfscreatecon_or_die(NULL);
- } else {
- bb_perror_msg("can't lgetfilecon %s", source);
- return -1;
- }
- }
-#endif
-
if (S_ISDIR(source_stat.st_mode)) {
DIR *dp;
const char *tp;
struct dirent *d;
mode_t saved_umask = 0;
- if (!(flags & FILEUTILS_RECUR)) {
- bb_error_msg("omitting directory '%s'", source);
- return -1;
- }
-
/* Did we ever create source ourself before? */
tp = is_in_ino_dev_hashtable(&source_stat);
if (tp) {
@@ -160,8 +130,6 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
saved_umask = umask(0);
mode = source_stat.st_mode;
- if (!(flags & FILEUTILS_PRESERVE_STATUS))
- mode = source_stat.st_mode & ~saved_umask;
/* Allow owner to access new dir (at least for now) */
mode |= S_IRWXU;
if (mkdir(dest, mode) < 0) {
@@ -210,45 +178,17 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
goto preserve_mode_ugid_time;
}
- if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) {
- int (*lf)(const char *oldpath, const char *newpath);
- make_links:
- /* Hmm... maybe
- * if (DEREF && MAKE_SOFTLINK) source = realpath(source) ?
- * (but realpath returns NULL on dangling symlinks...) */
- lf = (flags & FILEUTILS_MAKE_SOFTLINK) ? symlink : link;
- if (lf(source, dest) < 0) {
- ovr = ask_and_unlink(dest, flags);
- if (ovr <= 0)
- return ovr;
- if (lf(source, dest) < 0) {
- bb_perror_msg("can't create link '%s'", dest);
- return -1;
- }
- }
- /* _Not_ jumping to preserve_mode_ugid_time:
- * (sym)links don't have those */
- return 0;
- }
-
- if (/* "cp thing1 thing2" without -R: just open and read() from thing1 */
- !(flags & FILEUTILS_RECUR)
- /* "cp [-opts] regular_file thing2" */
- || S_ISREG(source_stat.st_mode)
- /* DEREF uses stat, which never returns S_ISLNK() == true.
- * So the below is never true: */
- /* || (FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) */
- ) {
+ if (S_ISREG(source_stat.st_mode) ) { /* "cp [-opts] regular_file thing2" */
int src_fd;
int dst_fd;
mode_t new_mode;
- if (!FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) {
+ if (S_ISLNK(source_stat.st_mode)) {
/* "cp -d symlink dst": create a link */
goto dont_cat;
}
- if (ENABLE_FEATURE_PRESERVE_HARDLINKS && !FLAGS_DEREF) {
+ if (ENABLE_FEATURE_PRESERVE_HARDLINKS) {
const char *link_target;
link_target = is_in_ino_dev_hashtable(&source_stat);
if (link_target) {
@@ -295,25 +235,6 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
}
}
-#if ENABLE_SELINUX
- if ((flags & (FILEUTILS_PRESERVE_SECURITY_CONTEXT|FILEUTILS_SET_SECURITY_CONTEXT))
- && is_selinux_enabled() > 0
- ) {
- security_context_t con;
- if (getfscreatecon(&con) == -1) {
- bb_perror_msg("getfscreatecon");
- return -1;
- }
- if (con) {
- if (setfilecon(dest, con) == -1) {
- bb_perror_msg("setfilecon:%s,%s", dest, con);
- freecon(con);
- return -1;
- }
- freecon(con);
- }
- }
-#endif
if (bb_copyfd_eof(src_fd, dst_fd) == -1)
retval = -1;
/* Careful with writing... */
@@ -348,9 +269,8 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
bb_perror_msg("can't create symlink '%s'", dest);
return -1;
}
- if (flags & FILEUTILS_PRESERVE_STATUS)
- if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
- bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+ if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
+ bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
}
/* _Not_ jumping to preserve_mode_ugid_time:
* symlinks don't have those */
@@ -370,10 +290,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
preserve_mode_ugid_time:
- if (flags & FILEUTILS_PRESERVE_STATUS
- /* Cannot happen: */
- /* && !(flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) */
- ) {
+ if (1 /*FILEUTILS_PRESERVE_STATUS*/) {
struct timeval times[2];
times[1].tv_sec = times[0].tv_sec = source_stat.st_mtime;
@@ -389,10 +306,6 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
}
- if (flags & FILEUTILS_VERBOSE) {
- printf("'%s' -> '%s'\n", source, dest);
- }
-
return retval;
}
#endif
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 01/11] copy.c: import copy_file() from busybox
From: Nguyễn Thái Ngọc Duy @ 2016-11-28 9:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161128094319.16176-1-pclouds@gmail.com>
This is busybox's unmodified copy_file() in libbb/copy_file.c from the
GPL2+ commit f2c043acfcf9dad9fd3d65821b81f89986bbe54e (busybox: fix
uninitialized memory when displaying IPv6 addresses -
2016-01-18). This is a no-op commit. More changes are needed before
this new code can compile.
This will be needed for implementing "git worktree move" where we have
to move a directory recursively. We can implement it from scratch, but
then we will have to deal with corner cases (failure to move, circular
symlinks...). And delegating the task to "/bin/mv" takes a way the
ability to clean things up properly when things fail and we may have
to deal with "mv" differences between platforms.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
copy.c | 331 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 331 insertions(+)
diff --git a/copy.c b/copy.c
index 4de6a11..79623ac 100644
--- a/copy.c
+++ b/copy.c
@@ -65,3 +65,334 @@ int copy_file_with_time(const char *dst, const char *src, int mode)
return copy_times(dst, src);
return status;
}
+
+#if 0
+/* Return:
+ * -1 error, copy not made
+ * 0 copy is made or user answered "no" in interactive mode
+ * (failures to preserve mode/owner/times are not reported in exit code)
+ */
+int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
+{
+ /* This is a recursive function, try to minimize stack usage */
+ /* NB: each struct stat is ~100 bytes */
+ struct stat source_stat;
+ struct stat dest_stat;
+ smallint retval = 0;
+ smallint dest_exists = 0;
+ smallint ovr;
+
+/* Inverse of cp -d ("cp without -d") */
+#define FLAGS_DEREF (flags & (FILEUTILS_DEREFERENCE + FILEUTILS_DEREFERENCE_L0))
+
+ if ((FLAGS_DEREF ? stat : lstat)(source, &source_stat) < 0) {
+ /* This may be a dangling symlink.
+ * Making [sym]links to dangling symlinks works, so... */
+ if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK))
+ goto make_links;
+ bb_perror_msg("can't stat '%s'", source);
+ return -1;
+ }
+
+ if (lstat(dest, &dest_stat) < 0) {
+ if (errno != ENOENT) {
+ bb_perror_msg("can't stat '%s'", dest);
+ return -1;
+ }
+ } else {
+ if (source_stat.st_dev == dest_stat.st_dev
+ && source_stat.st_ino == dest_stat.st_ino
+ ) {
+ bb_error_msg("'%s' and '%s' are the same file", source, dest);
+ return -1;
+ }
+ dest_exists = 1;
+ }
+
+#if ENABLE_SELINUX
+ if ((flags & FILEUTILS_PRESERVE_SECURITY_CONTEXT) && is_selinux_enabled() > 0) {
+ security_context_t con;
+ if (lgetfilecon(source, &con) >= 0) {
+ if (setfscreatecon(con) < 0) {
+ bb_perror_msg("can't set setfscreatecon %s", con);
+ freecon(con);
+ return -1;
+ }
+ } else if (errno == ENOTSUP || errno == ENODATA) {
+ setfscreatecon_or_die(NULL);
+ } else {
+ bb_perror_msg("can't lgetfilecon %s", source);
+ return -1;
+ }
+ }
+#endif
+
+ if (S_ISDIR(source_stat.st_mode)) {
+ DIR *dp;
+ const char *tp;
+ struct dirent *d;
+ mode_t saved_umask = 0;
+
+ if (!(flags & FILEUTILS_RECUR)) {
+ bb_error_msg("omitting directory '%s'", source);
+ return -1;
+ }
+
+ /* Did we ever create source ourself before? */
+ tp = is_in_ino_dev_hashtable(&source_stat);
+ if (tp) {
+ /* We did! it's a recursion! man the lifeboats... */
+ bb_error_msg("recursion detected, omitting directory '%s'",
+ source);
+ return -1;
+ }
+
+ if (dest_exists) {
+ if (!S_ISDIR(dest_stat.st_mode)) {
+ bb_error_msg("target '%s' is not a directory", dest);
+ return -1;
+ }
+ /* race here: user can substitute a symlink between
+ * this check and actual creation of files inside dest */
+ } else {
+ /* Create DEST */
+ mode_t mode;
+ saved_umask = umask(0);
+
+ mode = source_stat.st_mode;
+ if (!(flags & FILEUTILS_PRESERVE_STATUS))
+ mode = source_stat.st_mode & ~saved_umask;
+ /* Allow owner to access new dir (at least for now) */
+ mode |= S_IRWXU;
+ if (mkdir(dest, mode) < 0) {
+ umask(saved_umask);
+ bb_perror_msg("can't create directory '%s'", dest);
+ return -1;
+ }
+ umask(saved_umask);
+ /* need stat info for add_to_ino_dev_hashtable */
+ if (lstat(dest, &dest_stat) < 0) {
+ bb_perror_msg("can't stat '%s'", dest);
+ return -1;
+ }
+ }
+ /* remember (dev,inode) of each created dir.
+ * NULL: name is not remembered */
+ add_to_ino_dev_hashtable(&dest_stat, NULL);
+
+ /* Recursively copy files in SOURCE */
+ dp = opendir(source);
+ if (dp == NULL) {
+ retval = -1;
+ goto preserve_mode_ugid_time;
+ }
+
+ while ((d = readdir(dp)) != NULL) {
+ char *new_source, *new_dest;
+
+ new_source = concat_subpath_file(source, d->d_name);
+ if (new_source == NULL)
+ continue;
+ new_dest = concat_path_file(dest, d->d_name);
+ if (copy_file(new_source, new_dest, flags & ~FILEUTILS_DEREFERENCE_L0) < 0)
+ retval = -1;
+ free(new_source);
+ free(new_dest);
+ }
+ closedir(dp);
+
+ if (!dest_exists
+ && chmod(dest, source_stat.st_mode & ~saved_umask) < 0
+ ) {
+ bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
+ /* retval = -1; - WRONG! copy *WAS* made */
+ }
+ goto preserve_mode_ugid_time;
+ }
+
+ if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) {
+ int (*lf)(const char *oldpath, const char *newpath);
+ make_links:
+ /* Hmm... maybe
+ * if (DEREF && MAKE_SOFTLINK) source = realpath(source) ?
+ * (but realpath returns NULL on dangling symlinks...) */
+ lf = (flags & FILEUTILS_MAKE_SOFTLINK) ? symlink : link;
+ if (lf(source, dest) < 0) {
+ ovr = ask_and_unlink(dest, flags);
+ if (ovr <= 0)
+ return ovr;
+ if (lf(source, dest) < 0) {
+ bb_perror_msg("can't create link '%s'", dest);
+ return -1;
+ }
+ }
+ /* _Not_ jumping to preserve_mode_ugid_time:
+ * (sym)links don't have those */
+ return 0;
+ }
+
+ if (/* "cp thing1 thing2" without -R: just open and read() from thing1 */
+ !(flags & FILEUTILS_RECUR)
+ /* "cp [-opts] regular_file thing2" */
+ || S_ISREG(source_stat.st_mode)
+ /* DEREF uses stat, which never returns S_ISLNK() == true.
+ * So the below is never true: */
+ /* || (FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) */
+ ) {
+ int src_fd;
+ int dst_fd;
+ mode_t new_mode;
+
+ if (!FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) {
+ /* "cp -d symlink dst": create a link */
+ goto dont_cat;
+ }
+
+ if (ENABLE_FEATURE_PRESERVE_HARDLINKS && !FLAGS_DEREF) {
+ const char *link_target;
+ link_target = is_in_ino_dev_hashtable(&source_stat);
+ if (link_target) {
+ if (link(link_target, dest) < 0) {
+ ovr = ask_and_unlink(dest, flags);
+ if (ovr <= 0)
+ return ovr;
+ if (link(link_target, dest) < 0) {
+ bb_perror_msg("can't create link '%s'", dest);
+ return -1;
+ }
+ }
+ return 0;
+ }
+ add_to_ino_dev_hashtable(&source_stat, dest);
+ }
+
+ src_fd = open_or_warn(source, O_RDONLY);
+ if (src_fd < 0)
+ return -1;
+
+ /* Do not try to open with weird mode fields */
+ new_mode = source_stat.st_mode;
+ if (!S_ISREG(source_stat.st_mode))
+ new_mode = 0666;
+
+ // POSIX way is a security problem versus (sym)link attacks
+ if (!ENABLE_FEATURE_NON_POSIX_CP) {
+ dst_fd = open(dest, O_WRONLY|O_CREAT|O_TRUNC, new_mode);
+ } else { /* safe way: */
+ dst_fd = open(dest, O_WRONLY|O_CREAT|O_EXCL, new_mode);
+ }
+ if (dst_fd == -1) {
+ ovr = ask_and_unlink(dest, flags);
+ if (ovr <= 0) {
+ close(src_fd);
+ return ovr;
+ }
+ /* It shouldn't exist. If it exists, do not open (symlink attack?) */
+ dst_fd = open3_or_warn(dest, O_WRONLY|O_CREAT|O_EXCL, new_mode);
+ if (dst_fd < 0) {
+ close(src_fd);
+ return -1;
+ }
+ }
+
+#if ENABLE_SELINUX
+ if ((flags & (FILEUTILS_PRESERVE_SECURITY_CONTEXT|FILEUTILS_SET_SECURITY_CONTEXT))
+ && is_selinux_enabled() > 0
+ ) {
+ security_context_t con;
+ if (getfscreatecon(&con) == -1) {
+ bb_perror_msg("getfscreatecon");
+ return -1;
+ }
+ if (con) {
+ if (setfilecon(dest, con) == -1) {
+ bb_perror_msg("setfilecon:%s,%s", dest, con);
+ freecon(con);
+ return -1;
+ }
+ freecon(con);
+ }
+ }
+#endif
+ if (bb_copyfd_eof(src_fd, dst_fd) == -1)
+ retval = -1;
+ /* Careful with writing... */
+ if (close(dst_fd) < 0) {
+ bb_perror_msg("error writing to '%s'", dest);
+ retval = -1;
+ }
+ /* ...but read size is already checked by bb_copyfd_eof */
+ close(src_fd);
+ /* "cp /dev/something new_file" should not
+ * copy mode of /dev/something */
+ if (!S_ISREG(source_stat.st_mode))
+ return retval;
+ goto preserve_mode_ugid_time;
+ }
+ dont_cat:
+
+ /* Source is a symlink or a special file */
+ /* We are lazy here, a bit lax with races... */
+ if (dest_exists) {
+ errno = EEXIST;
+ ovr = ask_and_unlink(dest, flags);
+ if (ovr <= 0)
+ return ovr;
+ }
+ if (S_ISLNK(source_stat.st_mode)) {
+ char *lpath = xmalloc_readlink_or_warn(source);
+ if (lpath) {
+ int r = symlink(lpath, dest);
+ free(lpath);
+ if (r < 0) {
+ bb_perror_msg("can't create symlink '%s'", dest);
+ return -1;
+ }
+ if (flags & FILEUTILS_PRESERVE_STATUS)
+ if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
+ bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+ }
+ /* _Not_ jumping to preserve_mode_ugid_time:
+ * symlinks don't have those */
+ return 0;
+ }
+ if (S_ISBLK(source_stat.st_mode) || S_ISCHR(source_stat.st_mode)
+ || S_ISSOCK(source_stat.st_mode) || S_ISFIFO(source_stat.st_mode)
+ ) {
+ if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0) {
+ bb_perror_msg("can't create '%s'", dest);
+ return -1;
+ }
+ } else {
+ bb_error_msg("unrecognized file '%s' with mode %x", source, source_stat.st_mode);
+ return -1;
+ }
+
+ preserve_mode_ugid_time:
+
+ if (flags & FILEUTILS_PRESERVE_STATUS
+ /* Cannot happen: */
+ /* && !(flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) */
+ ) {
+ struct timeval times[2];
+
+ times[1].tv_sec = times[0].tv_sec = source_stat.st_mtime;
+ times[1].tv_usec = times[0].tv_usec = 0;
+ /* BTW, utimes sets usec-precision time - just FYI */
+ if (utimes(dest, times) < 0)
+ bb_perror_msg("can't preserve %s of '%s'", "times", dest);
+ if (chown(dest, source_stat.st_uid, source_stat.st_gid) < 0) {
+ source_stat.st_mode &= ~(S_ISUID | S_ISGID);
+ bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+ }
+ if (chmod(dest, source_stat.st_mode) < 0)
+ bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
+ }
+
+ if (flags & FILEUTILS_VERBOSE) {
+ printf("'%s' -> '%s'\n", source, dest);
+ }
+
+ return retval;
+}
+#endif
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 00/11] git worktree (re)move
From: Nguyễn Thái Ngọc Duy @ 2016-11-28 9:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161112022337.13317-1-pclouds@gmail.com>
v2 contains some style fix and adapts to the new get_worktrees() api
from nd/worktree-list-fixup (which means it can't be built without
that series).
Nguyễn Thái Ngọc Duy (11):
copy.c: import copy_file() from busybox
copy.c: delete unused code in copy_file()
copy.c: convert bb_(p)error_msg to error(_errno)
copy.c: style fix
copy.c: convert copy_file() to copy_dir_recursively()
worktree.c: add validate_worktree()
worktree.c: add update_worktree_location()
worktree move: new command
worktree move: accept destination as directory
worktree move: refuse to move worktrees with submodules
worktree remove: new command
Documentation/git-worktree.txt | 28 ++-
builtin/worktree.c | 181 ++++++++++++++++
cache.h | 1 +
contrib/completion/git-completion.bash | 5 +-
copy.c | 369 +++++++++++++++++++++++++++++++++
t/t2028-worktree-move.sh | 56 +++++
worktree.c | 84 ++++++++
worktree.h | 11 +
8 files changed, 724 insertions(+), 11 deletions(-)
--
2.8.2.524.g6ff3d78
^ permalink raw reply
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