* Re: Bug: stash staged file move loses original file deletion
From: Jeff King @ 2016-12-06 15:25 UTC (permalink / raw)
To: Matthew Patey; +Cc: git
In-Reply-To: <CAFQpxx+PJ3FSoH9DFWyEw+ZLagji9Qou+aY9EB8A+=t+QX0o2A@mail.gmail.com>
On Tue, Dec 06, 2016 at 02:45:05PM +0000, Matthew Patey wrote:
> Thanks for the reply! I agree that it is weird that applying a stash with a
> move only puts the addition back in the index, and thanks for the tip on
> "index" to properly apply that. For this case I was focusing on the
> behavior of the second stash/apply, with the first stash/apply as an
> example of how to get into a weird state that triggers the odd behavior of
> the second.
Oh, heh, I totally missed that.
I agree that the second stash not saving the deletion seems like a bug.
Oddly, just stashing a deletion, like:
rm a
git stash
git stash apply
does store it. So it's not like we can't represent the deletion. Somehow
the presence of "b" in the index matters.
It looks like the culprit may be this part of create_stash():
git diff --name-only -z HEAD -- >"$TMP-stagenames"
That is using the "git diff" porcelain, which will respect renames, and
the --name-only bit mentions only "b".
If you run:
git -c diff.renames=false stash
then it works.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Jeff King @ 2016-12-06 15:09 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1612061512190.117539@virtualbox>
On Tue, Dec 06, 2016 at 03:48:38PM +0100, Johannes Schindelin wrote:
> > Should it blindly look at ".git/config"?
>
> Absolutely not, of course. You did not need me to say that.
>
> > Now your program behaves differently depending on whether you are in the
> > top-level of the working tree.
>
> Exactly. This, BTW, is already how the code would behave if anybody called
> `git_path()` before `setup_git_directory()`, as the former function
> implicitly calls `setup_git_env()` which does *not* call
> `setup_git_directory()` but *does* set up `git_dir` which is then used by
> `do_git_config_sequence()`..
>
> We have a few of these nasty surprises in our code base, where code
> silently assumes that global state is set up correctly, and succeeds in
> sometimes surprising ways if it is not.
Right. I have been working on fixing this. v2.11 has a ton of tweaks in
this area, and my patch to die() rather than default to ".git" is
cooking in next to catch any stragglers.
> > Should it speculatively do repo discovery, and use any discovered config
> > file?
>
> Personally, I find the way we discover the repository most irritating. It
> seems that we have multiple, mutually incompatible code paths
> (`setup_git_directory()` and `setup_git_env()` come to mind already, and
> it does not help that consecutive calls to `setup_git_directory()` will
> yield a very unexpected outcome).
I agree. We should be killing off setup_git_env(), which is something
I've been slowly working towards over the years.
There are some annoyances with setup_git_directory(), too (like the fact
that you cannot ask "is there a git repository you can find" without
making un-recoverable changes to the process state). I think we should
fix those, too.
> > Now some commands respect config that they shouldn't (e.g., running "git
> > init foo.git" from inside another repository will accidentally pick up
> > the value of core.sharedrepository from wherever you happen to run it).
>
> Right. That points to another problem with the way we do things: we leak
> global state from discovering a git_dir, which means that we can neither
> undo nor override it.
>
> If we discovered our git_dir in a robust manner, `git init` could say:
> hey, this git_dir is actually not what I wanted, here is what I want.
>
> Likewise, `git submodule` would eventually be able to run in the very same
> process as the calling `git`, as would a local fetch.
Yep, I agree with all that. I do think things _have_ been improving over
the years, and the code is way less tangled than it used to be. But
there are so many corner cases, and the code is so fundamental, that it
has been slow going. I'd be happy to review patches if you want to push
it along.
> > So I think the caller of the config code has to provide some kind of
> > context about how it is expecting to run and how the value will be used.
>
> Yep.
>
> Maybe even go a step further and say that the config code needs a context
> "object".
If I were writing git from scratch, I'd consider making a "struct
repository" object. I'm not sure how painful it would be to retro-fit it
at this point.
> > Right now if setup_git_directory() or similar hasn't been called, the
> > config code does not look.
>
> Correct.
>
> Actually, half correct. If setup_git_directory() has not been called, but,
> say, git_path() (and thereby implicitly setup_git_env()), the config code
> *does* look. At a hard-coded .git/config.
Not since b9605bc4f (config: only read .git/config from configured
repos, 2016-09-12). That's why pager.c needs its little hack.
I guess you could see that as a step backwards, but I think it is
necessary one on the road to doing it right.
> > Ideally there would be a way for a caller to say "I am running early and
> > not even sure yet if we are in a repo; please speculatively try to find
> > repo config for me".
>
> And ideally, it would not roll *yet another* way to discover the git_dir,
> but it would reuse the same function (fixing it not to chdir()
> unilaterally).
Yes, absolutely.
> Of course, not using `chdir()` means that we have to figure out symbolic
> links somehow, in case somebody works from a symlinked subdirectory, e.g.:
>
> ln -s $PWD/t/ ~/test-directory
> cd ~/test-directory
> git log
There's work happening elsewhere[1] on making real_path() work without
calling chdir(). Which necessarily involves resolving symlinks
ourselves. I wonder if we could leverage that work here (ideally by
using real_path() under the hood, and not reimplementing the same
readlink() logic ourselves).
[1] http://public-inbox.org/git/1480964316-99305-1-git-send-email-bmwill@google.com/
> > The pager code does this manually, and without great accuracy; see the
> > hack in pager.c's read_early_config().
>
> I saw it. And that is what triggered the mail you are responding to (you
> probably saw my eye-rolling between the lines).
>
> The real question is: can we fix this? Or is there simply too great
> reluctance to change the current code?
The code in pager.c is only a month or two old. Like I said, it's ugly,
but I think it's a necessary step on the way forward. So I don't think
there's reluctance at all. The next steps (which I outlined) just
haven't been taken yet.
> > I think the way forward is:
> >
> > 1. Make that an optional behavior in git_config_with_options() so
> > other spots can reuse it (probably alias lookup, and something like
> > your difftool config).
>
> Ideally: *any* early call to `git_config_get_value()`. Make things less
> surprising.
I'm not convinced that's a good idea. The changes in b9605bc4f were
motivated by a real bug, which your suggestion would reintroduce (namely
low-level code run by git-init ending up with config variables from a
repo that _should_ be unrelated).
In my mental model, the cases are:
1. We are "early" in the process, before we know if we have a repo or
not. These early looks should speculatively look at repo config,
which is confined to generic things like pager config, alias
config, etc.
2. We are in a repo. Obviously look at $GIT_DIR/config.
3. We are in a program which has done setup and determined we are
_not_ in a repo. Definitely do not look at .git/config or anything
else.
My plan was for the config code to default to (3) when we are not in a
repo, but let some lookups ask specifically for (1).
If you want to default to (1), you need some way for programs to say "I
am really case (3); do not look for a repo". And it needs to be global,
as the config lookup may be done by much lower-level code. That could be
by turning startup_info->have_repository into a tri-state. It just
wasn't the way I was planning on it.
> > 2. Make it more accurate. Right now it blindly looks in .git/config,
> > but it should be able to do the usual repo-detection (_without_
> > actually entering the repo) to try to find a possible config file.
>
> The real trick will be to convince Junio to have a single function for
> git_dir discovery, I guess, lest we have multiple, slightly incompatible
> ways to discover it. I expect a lot of resistance here, because we would
> have to change tried-and-tested (if POLA-violating) code.
Personally, I haven't seen any resistance from Junio on refactoring this
area. I'm sure he is concerned that we do not regress, but it's not like
the area has been unchanged over the years. It has been slow going
because we want to do it carefully, but I think we are actually at the
point now where the next step is making setup_git_directory() more sane.
-Peff
^ permalink raw reply
* inconsistent output from git add about ignored files
From: Yaroslav Halchenko @ 2016-12-06 14:18 UTC (permalink / raw)
To: git; +Cc: Benjamin Poldrack
Dear Git gurus,
It seems that there is some inconsistency in output of git while it is
ignoring files:
if a single path within ignored directory is provided -- git outputs
the file path. If multiple files are provided (some of which aren't
ignored) -- git outputs only the ignored directory name:
% git --version
git version 2.10.2
% git status
On branch master
Untracked files:
(use "git add <file>..." to include in what will be committed)
bu
nothing added to commit but untracked files present (use "git add" to track)
% cat .gitignore
*.me
% git add blah.me/bu
The following paths are ignored by one of your .gitignore files:
blah.me/bu
Use -f if you really want to add them.
% git add blah.me/bu bu
The following paths are ignored by one of your .gitignore files:
blah.me
Use -f if you really want to add them.
% git status
On branch master
Changes to be committed:
(use "git reset HEAD <file>..." to unstage)
new file: bu
So note that in the first case it reports "blah.me/bu" whenever in the
second -- only "blah.me".
P.S. Please CC us in your replies.
With best regards,
--
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2016-12-06 14:48 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <20161206133650.t7gkg4f6wzw3zxki@sigill.intra.peff.net>
Hi Peff,
On Tue, 6 Dec 2016, Jeff King wrote:
> On Tue, Dec 06, 2016 at 02:16:35PM +0100, Johannes Schindelin wrote:
>
> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > >
> > > > Seriously, do you really think it is a good idea to have
> > > > git_config_get_value() *ignore* any value in .git/config?
> > >
> > > When we do not know ".git/" directory we see is the repository we
> > > were told to work on, then we should ignore ".git/config" file. So
> > > allowing git_config_get_value() to _ignore_ ".git/config" before the
> > > program calls setup_git_directory() does have its uses.
> >
> > I think you are wrong. This is yet another inconsistent behavior that
> > violates the Law of Least Surprises.
>
> There are surprises in this code any way you turn. If we have not
> called setup_git_directory(), then how does git_config_get_value() know
> if we are in a repository or not?
My biggest surprise, frankly, would be that `git init` and `git clone` are
not special-cased.
> Should it blindly look at ".git/config"?
Absolutely not, of course. You did not need me to say that.
> Now your program behaves differently depending on whether you are in the
> top-level of the working tree.
Exactly. This, BTW, is already how the code would behave if anybody called
`git_path()` before `setup_git_directory()`, as the former function
implicitly calls `setup_git_env()` which does *not* call
`setup_git_directory()` but *does* set up `git_dir` which is then used by
`do_git_config_sequence()`..
We have a few of these nasty surprises in our code base, where code
silently assumes that global state is set up correctly, and succeeds in
sometimes surprising ways if it is not.
> Should it speculatively do repo discovery, and use any discovered config
> file?
Personally, I find the way we discover the repository most irritating. It
seems that we have multiple, mutually incompatible code paths
(`setup_git_directory()` and `setup_git_env()` come to mind already, and
it does not help that consecutive calls to `setup_git_directory()` will
yield a very unexpected outcome).
Just try to explain to a veteran software engineer why you cannot call
`setup_git_directory_gently()` multiple times and expect the very same
return value every time.
Another irritation is that some commands that clearly would like to use a
repository if there is one (such as `git diff`) are *not* marked with
RUN_SETUP_GENTLY, due to these unfortunate implementation details.
> Now some commands respect config that they shouldn't (e.g., running "git
> init foo.git" from inside another repository will accidentally pick up
> the value of core.sharedrepository from wherever you happen to run it).
Right. That points to another problem with the way we do things: we leak
global state from discovering a git_dir, which means that we can neither
undo nor override it.
If we discovered our git_dir in a robust manner, `git init` could say:
hey, this git_dir is actually not what I wanted, here is what I want.
Likewise, `git submodule` would eventually be able to run in the very same
process as the calling `git`, as would a local fetch.
> So I think the caller of the config code has to provide some kind of
> context about how it is expecting to run and how the value will be used.
Yep.
Maybe even go a step further and say that the config code needs a context
"object".
> Right now if setup_git_directory() or similar hasn't been called, the
> config code does not look.
Correct.
Actually, half correct. If setup_git_directory() has not been called, but,
say, git_path() (and thereby implicitly setup_git_env()), the config code
*does* look. At a hard-coded .git/config.
> Ideally there would be a way for a caller to say "I am running early and
> not even sure yet if we are in a repo; please speculatively try to find
> repo config for me".
And ideally, it would not roll *yet another* way to discover the git_dir,
but it would reuse the same function (fixing it not to chdir()
unilaterally).
Of course, not using `chdir()` means that we have to figure out symbolic
links somehow, in case somebody works from a symlinked subdirectory, e.g.:
ln -s $PWD/t/ ~/test-directory
cd ~/test-directory
git log
> The pager code does this manually, and without great accuracy; see the
> hack in pager.c's read_early_config().
I saw it. And that is what triggered the mail you are responding to (you
probably saw my eye-rolling between the lines).
The real question is: can we fix this? Or is there simply too great
reluctance to change the current code?
> I think the way forward is:
>
> 1. Make that an optional behavior in git_config_with_options() so
> other spots can reuse it (probably alias lookup, and something like
> your difftool config).
Ideally: *any* early call to `git_config_get_value()`. Make things less
surprising.
> 2. Make it more accurate. Right now it blindly looks in .git/config,
> but it should be able to do the usual repo-detection (_without_
> actually entering the repo) to try to find a possible config file.
The real trick will be to convince Junio to have a single function for
git_dir discovery, I guess, lest we have multiple, slightly incompatible
ways to discover it. I expect a lot of resistance here, because we would
have to change tried-and-tested (if POLA-violating) code.
Ciao,
Dscho
^ permalink raw reply
* Re: Bug: stash staged file move loses original file deletion
From: Jeff King @ 2016-12-06 14:24 UTC (permalink / raw)
To: Matthew Patey; +Cc: git
In-Reply-To: <CAFQpxxKbn4vBMzVcLZgBVvuL2fsOGNMHR1WC+aTOG_RAWkZ_Gg@mail.gmail.com>
On Mon, Dec 05, 2016 at 09:37:51AM -0500, Matthew Patey wrote:
> Git version 2.8.0 (sorry can't update to more recent on my machine) on Ubuntu.
The behavior is the same on more recent versions of git, too. The short
answer is "use --index". The longer one is below.
> After moving a file, if the new file is in the index but the deletion
> of the old file is not staged, git stash loses the deletion operation.
> Repro:
>
> 1. git mv a b
> This will have both the "deletion" and the "file added" in the index
>
> 2. git stash; git stash pop
> Now file a is still in the index, but file b deletion is not staged.
>
> 3. git stash; git stash show -v
> This will show that the deletion operation is not in the stash
>
> 4. git stash pop
> Again confirms the issue, file a is in the index, but file b is
> present and unmodified in the working directory.
Thanks for a clear reproduction case. I think the oddball, though, is
not that "b" is not staged for deletion, but that the addition of "a"
_is_ staged.
Applying a stash usually does not re-stage index contents, unless you
specify --index. For example, try:
# Make a staged change
echo change >>a
git add a
# This puts the change back into the working tree, but does _not_
# put it into the index.
git stash apply
# Now reset to try again.
git reset --hard
# This does restore the index.
git stash apply --index
So in your case, the deletion of "b" is following that same rule. What's
unusual is that "a" is staged. There's code specifically in git-stash
specifically to make sure this is the case, but I don't remember offhand
why this is so. The code comes from the original f2c66ed19 (Add
git-stash script, 2007-06-30), which in turn seems to come from Junio's
comments in:
http://public-inbox.org/git/7vmyyq2zrz.fsf@assigned-by-dhcp.pobox.com/
I don't see any specific reasoning, but I think it is simply that it is
confusing for the files to become untracked totally. These days we have
intent-to-add via "git add -N", so that might actually be a better
choice for this case.
Anyway, that history digression is neither here nor there for your case.
If you want to restore the index contents, use "--index". That does what
you were expecting:
$ git mv a b
$ git stash && git stash apply --index
Saved working directory and index state WIP on master: 5355755 foo
HEAD is now at 5355755 foo
On branch master
Changes to be committed:
renamed: a -> b
-Peff
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Jeff King @ 2016-12-06 14:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, GIT Mailing-list
In-Reply-To: <xmqqbmwq5k7k.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 05, 2016 at 10:01:19AM -0800, Junio C Hamano wrote:
> > That said, I think the right patch may be to just drop --abbrev
> > entirely.
> > ...
> > I think at that point it was a noop, as 7 should have been the default.
> > And now we probably ought to drop it, so that we can use the
> > auto-scaling default.
>
> Yeah, I agree.
>
> It does mean that snapshot binaries built out of the same commit in
> the same repository before and after a repack have higher chances of
> getting named differently, which may surprise people, but that
> already is possible with a fixed length if the repacking involves
> pruning (albeit with lower probabilities), and I do not think it is
> a problem.
I think that the number is already unstable, even with --abbrev, as it
just specifies a minimum. So any operation that creates objects has a
possibility of increasing the length. Or more likely, two people
describing the same version may end up with different strings because
they have different objects in their repositories (e.g., I used to
carry's trast's git-notes archive of the mailing list which added quite
a few objects).
I agree that having it change over the course of a repack is slightly
_more_ surprising than those cases, but ultimately the value just isn't
stable.
-Peff
^ permalink raw reply
* Re: git add -p doesn't honor diff.noprefix config
From: Jeff King @ 2016-12-06 13:56 UTC (permalink / raw)
To: paddor; +Cc: git
In-Reply-To: <E1D7329A-A54B-4D09-A72A-62ECA8005752@gmail.com>
On Sat, Dec 03, 2016 at 07:45:18AM +0100, paddor wrote:
> I set the config diff.noprefix = true because I don't like the a/ and
> b/ prefixes, which nicely changed the output of `git diff`.
> Unfortunately, the filenames in the output of `git add --patch` are
> still prefixed.
>
> To me, this seems like a bug. Or there's a config option missing.
The interactive-add process is a perl script built around plumbing
commands like diff-tree, diff-files, etc. Plumbing commands do not
respect some config options, so that the output remains stable or
scripts built around them. And diff.noprefix is one of these. So scripts
have to get the value themselves and decide whether to pass it along to
the plumbing.
In this case, I think there are two steps needed:
1. Confirm that git-add--interactive.perl can actually handle
no-prefix patches. It feeds the patches back to git-apply, which
may be a complication (so it may need, for example, to pass a
special flag to git-apply).
2. git-add--interactive.perl needs to parse the config value, and if
set, pass the appropriate option to the diff plumbing. This should
only be one or two lines; see how $diff_algorithm is handled in
that script.
-Peff
^ permalink raw reply
* Re: [PATCH v2 0/6] shallow.c improvements
From: Duy Nguyen @ 2016-12-06 13:47 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Rasmus Villemoes
In-Reply-To: <20161206134212.mttcb75dov2jvqu5@sigill.intra.peff.net>
On Tue, Dec 6, 2016 at 8:42 PM, Jeff King <peff@peff.net> wrote:
> The final one _seems_ reasonable after reading your explanation, but I
> lack enough context to know whether or not there might be a corner case
> that you're missing. I'm inclined to trust your assessment on it.
Yeah I basically just wrote down my thoughts so somebody could maybe
spot something wrong. I'm going to think about it some more in the
next few days.
--
Duy
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Jeff King @ 2016-12-06 13:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <xmqqr35m3zx7.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 05, 2016 at 12:04:52PM -0800, Junio C Hamano wrote:
> > I'm sending out another reroll of this series so that in Jeff's he can
> > just call 'get_curl_allowed_protocols(-1)' for the non-redirection curl
> > option, which should make this test stop barfing.
>
> I was hoping to eventually merge Peff's series to older maintenance
> tracks. How bad would it be if we rebased the v8 of this series
> together with Peff's series to say v2.9 (or even older if it does
> not look too bad)?
My series actually fixes existing security problems, so I'd consider it
a bug-fix. I _think_ Brandon's series is purely about allowing more
expressiveness in the whitelist policy, and so could be considered more
of a feature.
So one option is to apply my series for older 'maint', and then just
rebase Brandon's on top of that for 'master'.
I don't know if that makes things any easier. I feel funny saying "no,
no, mine preempts yours because it is more maint-worthy", but I think
that order does make sense.
I think it would be OK to put Brandon's on maint, too, though. It is a
refactor of an existing security feature to make it more featureful, but
the way it is implemented could not cause security regressions unless
you use the new feature (IOW, we still respect the whitelist environment
exactly as before).
-Peff
^ permalink raw reply
* Re: [PATCH] difftool: fix dir-diff index creation when in a subdirectory
From: Johannes Schindelin @ 2016-12-06 13:46 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, Git ML, Frank Becker, John Keeping
In-Reply-To: <20161205222600.29914-1-davvid@gmail.com>
Hi David,
On Mon, 5 Dec 2016, David Aguilar wrote:
> 9ec26e797781239b36ebccb87c590e5778358007 corrected how path arguments
How about using the "whatis" instead, i.e. "9ec26e7977 (difftool: fix
argument handling in subdirs, 2016-07-18)"?
Ciao,
Dscho
^ permalink raw reply
* [PATCH v2 0/6] shallow.c improvements
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <1480710664-26290-1-git-send-email-rv@rasmusvillemoes.dk>
After staring not-so-hard and not-for-so-long at the code. This is
what I come up with. Rasmus I replaced two of your commits with my
own (and thank you for giving me an opportunity to refresh my memory
with this stuff). The first two commits are new and the result of
Jeff's observation on COMMIT_SLAB_SIZE.
You may find the description here a bit different from my explanation
previously (about "exclude/shallow requests"). Well.. I was wrong.
I had the recent --exclude-tag and friends in mind, but this is about
clone/fetch/push from/to a shallow repository since 2013, no wonder I
don't remember much about it :-D
Nguyễn Thái Ngọc Duy (4):
shallow.c: rename fields in paint_info to better express their purposes
shallow.c: stop abusing COMMIT_SLAB_SIZE for paint_info's memory pools
shallow.c: make paint_alloc slightly more robust
shallow.c: remove useless code
Rasmus Villemoes (2):
shallow.c: avoid theoretical pointer wrap-around
shallow.c: bit manipulation tweaks
shallow.c | 39 ++++++++++++++++++++-------------------
1 file changed, 20 insertions(+), 19 deletions(-)
--
2.8.2.524.g6ff3d78
^ permalink raw reply
* Re: [PATCH v2 0/6] shallow.c improvements
From: Jeff King @ 2016-12-06 13:42 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, rv
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
On Tue, Dec 06, 2016 at 07:53:33PM +0700, Nguyễn Thái Ngọc Duy wrote:
> Nguyễn Thái Ngọc Duy (4):
> shallow.c: rename fields in paint_info to better express their purposes
> shallow.c: stop abusing COMMIT_SLAB_SIZE for paint_info's memory pools
> shallow.c: make paint_alloc slightly more robust
> shallow.c: remove useless code
>
> Rasmus Villemoes (2):
> shallow.c: avoid theoretical pointer wrap-around
> shallow.c: bit manipulation tweaks
The first 5 patches look obviously good to me. The naming changes in
paint_alloc() make things much clearer, and the fixes retained from
Rasmus are all obvious improvements.
The final one _seems_ reasonable after reading your explanation, but I
lack enough context to know whether or not there might be a corner case
that you're missing. I'm inclined to trust your assessment on it.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Jeff King @ 2016-12-06 13:36 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1612061411000.117539@virtualbox>
On Tue, Dec 06, 2016 at 02:16:35PM +0100, Johannes Schindelin wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > > Seriously, do you really think it is a good idea to have
> > > git_config_get_value() *ignore* any value in .git/config?
> >
> > When we do not know ".git/" directory we see is the repository we were
> > told to work on, then we should ignore ".git/config" file. So allowing
> > git_config_get_value() to _ignore_ ".git/config" before the program
> > calls setup_git_directory() does have its uses.
>
> I think you are wrong. This is yet another inconsistent behavior that
> violates the Law of Least Surprises.
There are surprises in this code any way you turn. If we have not
called setup_git_directory(), then how does git_config_get_value() know
if we are in a repository or not?
Should it blindly look at ".git/config"? Now your program behaves
differently depending on whether you are in the top-level of the working
tree.
Should it speculatively do repo discovery, and use any discovered config
file? Now some commands respect config that they shouldn't (e.g.,
running "git init foo.git" from inside another repository will
accidentally pick up the value of core.sharedrepository from wherever
you happen to run it).
So I think the caller of the config code has to provide some kind of
context about how it is expecting to run and how the value will be used.
Right now if setup_git_directory() or similar hasn't been called, the
config code does not look. Ideally there would be a way for a caller to
say "I am running early and not even sure yet if we are in a repo;
please speculatively try to find repo config for me".
The pager code does this manually, and without great accuracy; see the
hack in pager.c's read_early_config(). I think the way forward is:
1. Make that an optional behavior in git_config_with_options() so
other spots can reuse it (probably alias lookup, and something like
your difftool config).
2. Make it more accurate. Right now it blindly looks in .git/config,
but it should be able to do the usual repo-detection (_without_
actually entering the repo) to try to find a possible config file.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2016-12-06 13:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <xmqqy3zu43yk.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 5 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Seriously, do you really think it is a good idea to have
> > git_config_get_value() *ignore* any value in .git/config?
>
> When we do not know ".git/" directory we see is the repository we were
> told to work on, then we should ignore ".git/config" file. So allowing
> git_config_get_value() to _ignore_ ".git/config" before the program
> calls setup_git_directory() does have its uses.
I think you are wrong. This is yet another inconsistent behavior that
violates the Law of Least Surprises.
> > We need to fix this.
>
> I have a feeling that "environment modifications that cannot be undone"
> we used as the rationale in 73c2779f42 ("builtin-am: implement skeletal
> builtin am", 2015-08-04) might be overly pessimistic and in order to
> implement undo_setup_git_directory(), all we need to do may just be
> matter of doing a chdir(2) back to prefix and unsetting GIT_PREFIX
> environment, but I haven't looked into details of the setup sequence
> recently.
The problem is that we may not know enough anymore to "undo
setup_git_directory()", as some environment variables may have been set
before calling Git. If we simply unset the environment variables, we do it
incorrectly. On the other hand, the environment variables *may* have been
set by setup_git_directory(). In which case we *do* have to unset them.
The entire setup_git_directory() logic is a bit of a historically-grown
garden.
Ciao,
Dscho
^ permalink raw reply
* [PATCH v2 2/6] shallow.c: stop abusing COMMIT_SLAB_SIZE for paint_info's memory pools
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
We need to allocate a "big" block of memory in paint_alloc(). The exact
size does not really matter. But the pool size has no relation with
commit-slab. Stop using that macro here.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
shallow.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/shallow.c b/shallow.c
index 8100dfd..2512ed3 100644
--- a/shallow.c
+++ b/shallow.c
@@ -431,6 +431,8 @@ void remove_nonexistent_theirs_shallow(struct shallow_info *info)
define_commit_slab(ref_bitmap, uint32_t *);
+#define POOL_SIZE (512 * 1024)
+
struct paint_info {
struct ref_bitmap ref_bitmap;
unsigned nr_bits;
@@ -447,9 +449,9 @@ static uint32_t *paint_alloc(struct paint_info *info)
if (!info->pool_count || info->free + size > info->end) {
info->pool_count++;
REALLOC_ARRAY(info->pools, info->pool_count);
- info->free = xmalloc(COMMIT_SLAB_SIZE);
+ info->free = xmalloc(POOL_SIZE);
info->pools[info->pool_count - 1] = info->free;
- info->end = info->free + COMMIT_SLAB_SIZE;
+ info->end = info->free + POOL_SIZE;
}
p = info->free;
info->free += size;
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 3/6] shallow.c: make paint_alloc slightly more robust
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
paint_alloc() allocates a big block of memory and splits it into
smaller, fixed size, chunks of memory whenever it's called. Each chunk
contains enough bits to present all "new refs" [1] in a fetch from a
shallow repository.
We do not check if the new "big block" is smaller than the requested
memory chunk though. If it happens, we'll happily pass back a memory
region smaller than expected. Which will lead to problems eventually.
A normal fetch may add/update a dozen new refs. Let's stay on the
"reasonably extreme" side and say we need 16k refs (or bits from
paint_alloc's perspective). Each chunk of memory would be 2k, much
smaller than the memory pool (512k).
So, normally, the under-allocation situation should never happen. A bad
guy, however, could make a fetch that adds more than 4m new/updated refs
to this code which results in a memory chunk larger than pool size.
Check this case and abort.
Noticed-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
[1] Details are in commit message of 58babff (shallow.c: the 8 steps to
select new commits for .git/shallow - 2013-12-05), step 6.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
shallow.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/shallow.c b/shallow.c
index 2512ed3..75e1702 100644
--- a/shallow.c
+++ b/shallow.c
@@ -447,6 +447,9 @@ static uint32_t *paint_alloc(struct paint_info *info)
unsigned size = nr * sizeof(uint32_t);
void *p;
if (!info->pool_count || info->free + size > info->end) {
+ if (size > POOL_SIZE)
+ die("BUG: pool size too small for %d in paint_alloc()",
+ size);
info->pool_count++;
REALLOC_ARRAY(info->pools, info->pool_count);
info->free = xmalloc(POOL_SIZE);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 6/6] shallow.c: remove useless code
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
Some context before we talk about the removed code.
This paint_down() is part of step 6 of 58babff (shallow.c: the 8 steps
to select new commits for .git/shallow - 2013-12-05). When we fetch from
a shallow repository, we need to know if one of the new/updated refs
needs new "shallow commits" in .git/shallow (because we don't have
enough history of those refs) and which one.
The question at step 6 is, what (new) shallow commits are required in
other to maintain reachability throughout the repository _without_
cutting our history short? To answer, we mark all commits reachable from
existing refs with UNINTERESTING ("rev-list --not --all"), mark shallow
commits with BOTTOM, then for each new/updated refs, walk through the
commit graph until we either hit UNINTERESTING or BOTTOM, marking the
ref on the commit as we walk.
After all the walking is done, we check the new shallow commits. If we
have not seen any new ref marked on a new shallow commit, we know all
new/updated refs are reachable using just our history and .git/shallow.
The shallow commit in question is not needed and can be thrown away.
So, the code.
The loop here (to walk through commits) is basically
1. get one commit from the queue
2. ignore if it's SEEN or UNINTERESTING
3. mark it
4. go through all the parents and..
5a. mark it if it's never marked before
5b. put it back in the queue
What we do in this patch is drop step 5a because it is not
necessary. The commit being marked at 5a is put back on the queue, and
will be marked at step 3 at the next iteration. The only case it will
not be marked is when the commit is already marked UNINTERESTING (5a
does not check this), which will be ignored at step 2.
But we don't care about refs marking on UNINTERESTING. We care about the
marking on _shallow commits_ that are not reachable from our current
history (and having UNINTERESTING on it means it's reachable). So it's
ok for an UNINTERESTING not to be ref-marked.
Reported-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
shallow.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/shallow.c b/shallow.c
index beb967e..11f7dde 100644
--- a/shallow.c
+++ b/shallow.c
@@ -512,12 +512,8 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
oid_to_hex(&c->object.oid));
for (p = c->parents; p; p = p->next) {
- uint32_t **p_refs = ref_bitmap_at(&info->ref_bitmap,
- p->item);
if (p->item->object.flags & SEEN)
continue;
- if (*p_refs == NULL || *p_refs == *refs)
- *p_refs = *refs;
commit_list_insert(p->item, &head);
}
}
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 4/6] shallow.c: avoid theoretical pointer wrap-around
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
From: Rasmus Villemoes <rv@rasmusvillemoes.dk>
The expression info->free+size is technically undefined behaviour in
exactly the case we want to test for. Moreover, the compiler is likely
to translate the expression to
(unsigned long)info->free + size > (unsigned long)info->end
where there's at least a theoretical chance that the LHS could wrap
around 0, giving a false negative.
This might as well be written using pointer subtraction avoiding these
issues.
Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
shallow.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/shallow.c b/shallow.c
index 75e1702..719f699 100644
--- a/shallow.c
+++ b/shallow.c
@@ -446,7 +446,7 @@ static uint32_t *paint_alloc(struct paint_info *info)
unsigned nr = (info->nr_bits + 31) / 32;
unsigned size = nr * sizeof(uint32_t);
void *p;
- if (!info->pool_count || info->free + size > info->end) {
+ if (!info->pool_count || size > info->end - info->free) {
if (size > POOL_SIZE)
die("BUG: pool size too small for %d in paint_alloc()",
size);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 5/6] shallow.c: bit manipulation tweaks
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
From: Rasmus Villemoes <rv@rasmusvillemoes.dk>
First of all, 1 << 31 is technically undefined behaviour, so let's just
use an unsigned literal.
If i is 'signed int' and gcc doesn't know that i is positive, gcc
generates code to compute the C99-mandated values of "i / 32" and "i %
32", which is a lot more complicated than simple a simple shifts/mask.
The only caller of paint_down actually passes an "unsigned int" value,
but the prototype of paint_down causes (completely well-defined)
conversion to signed int, and gcc has no way of knowing that the
converted value is non-negative. Just make the id parameter unsigned.
In update_refstatus, the change in generated code is much smaller,
presumably because gcc is smart enough to see that i starts as 0 and is
only incremented, so it is allowed (per the UD of signed overflow) to
assume that i is always non-negative. But let's just help less smart
compilers generate good code anyway.
Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
shallow.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/shallow.c b/shallow.c
index 719f699..beb967e 100644
--- a/shallow.c
+++ b/shallow.c
@@ -467,7 +467,7 @@ static uint32_t *paint_alloc(struct paint_info *info)
* all walked commits.
*/
static void paint_down(struct paint_info *info, const unsigned char *sha1,
- int id)
+ unsigned int id)
{
unsigned int i, nr;
struct commit_list *head = NULL;
@@ -479,7 +479,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
if (!c)
return;
memset(bitmap, 0, bitmap_size);
- bitmap[id / 32] |= (1 << (id % 32));
+ bitmap[id / 32] |= (1U << (id % 32));
commit_list_insert(c, &head);
while (head) {
struct commit_list *p;
@@ -653,11 +653,11 @@ static int add_ref(const char *refname, const struct object_id *oid,
static void update_refstatus(int *ref_status, int nr, uint32_t *bitmap)
{
- int i;
+ unsigned int i;
if (!ref_status)
return;
for (i = 0; i < nr; i++)
- if (bitmap[i / 32] & (1 << (i % 32)))
+ if (bitmap[i / 32] & (1U << (i % 32)))
ref_status[i]++;
}
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 1/6] shallow.c: rename fields in paint_info to better express their purposes
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
paint_alloc() is basically malloc(), tuned for allocating a fixed number
of bits on every call without worrying about freeing any individual
allocation since all will be freed at the end. It does it by allocating
a big block of memory every time it runs out of "free memory". "slab" is
a poor choice of name, at least poorer than "pool".
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
shallow.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/shallow.c b/shallow.c
index 4d0b005..8100dfd 100644
--- a/shallow.c
+++ b/shallow.c
@@ -434,9 +434,9 @@ define_commit_slab(ref_bitmap, uint32_t *);
struct paint_info {
struct ref_bitmap ref_bitmap;
unsigned nr_bits;
- char **slab;
+ char **pools;
char *free, *end;
- unsigned slab_count;
+ unsigned pool_count;
};
static uint32_t *paint_alloc(struct paint_info *info)
@@ -444,11 +444,11 @@ static uint32_t *paint_alloc(struct paint_info *info)
unsigned nr = (info->nr_bits + 31) / 32;
unsigned size = nr * sizeof(uint32_t);
void *p;
- if (!info->slab_count || info->free + size > info->end) {
- info->slab_count++;
- REALLOC_ARRAY(info->slab, info->slab_count);
+ if (!info->pool_count || info->free + size > info->end) {
+ info->pool_count++;
+ REALLOC_ARRAY(info->pools, info->pool_count);
info->free = xmalloc(COMMIT_SLAB_SIZE);
- info->slab[info->slab_count - 1] = info->free;
+ info->pools[info->pool_count - 1] = info->free;
info->end = info->free + COMMIT_SLAB_SIZE;
}
p = info->free;
@@ -624,9 +624,9 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,
post_assign_shallow(info, &pi.ref_bitmap, ref_status);
clear_ref_bitmap(&pi.ref_bitmap);
- for (i = 0; i < pi.slab_count; i++)
- free(pi.slab[i]);
- free(pi.slab);
+ for (i = 0; i < pi.pool_count; i++)
+ free(pi.pools[i]);
+ free(pi.pools);
free(shallow);
}
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* Re: [PATCH] commit: make --only --allow-empty work without paths
From: Andreas Krey @ 2016-12-06 9:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqh96i3ygs.fsf@gitster.mtv.corp.google.com>
On Mon, 05 Dec 2016 12:36:19 +0000, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> > On Sat, Dec 03, 2016 at 07:59:49AM +0100, Andreas Krey wrote:
...
> >> Tool: git commit --allow-empty -m 'FIX: A-123'
> >
> > OK. I think "tool" is slightly funny here, but I get that is part of the
> > real world works. Thanks for illustrating.
>
> I am not sure if I understand. Why isn't the FIX: thing added to
> the commit being pulled by amending it?
Because we don't allow push -f on our blessed repo (bitbucket).
(Oops, answer to wrong question. But the integrators don't want
to meddle with dev's commits, either.)
This has multiple reasons:
- The percentage of people who can and would be willing
to do rebase -i is small. (Not that they are likely to
increase under this policy.)
- Our build tool record builds by commit id, and when
you rebase (even if only for commit message edits)
you lose your (simple) build history.
> Would the convention be for
> the responder of a pull-request to fetch and drop the tip commit?
No, they need to keep it as there is automation hinging on the FIX line.
I would much prefer people to do rebases/amends instead of this crutch,
but that's not for now.
Hmm, it just occurred to me that we might allow force pushes for specific
users to keep the foot-shooting ratio low.
- Andreas
--
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800
^ permalink raw reply
* Re: [PATCH] clone,fetch: explain the shallow-clone option a little more clearly
From: Duy Nguyen @ 2016-12-06 9:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alex Henrie, Git Mailing List
In-Reply-To: <xmqq37i25iy5.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 6, 2016 at 1:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I however offhand do not think the feature can be used to make the
> repository shallower
I'm pretty sure it can, though it's a waste because you should be able
to shorten your history without talking to a remote server. But that
no-remote shortening is not implemented yet. And you probably want an
option to check with remote anyway to make sure the part you cut out
is available there, no history will be lost.
--
Duy
^ permalink raw reply
* Re: [PATCH] git-p4: add p4 shelf support
From: Luke Diamand @ 2016-12-06 8:36 UTC (permalink / raw)
To: Nuno Subtil; +Cc: Git Users, Junio C Hamano, Vinicius Kursancew, Lars Schneider
In-Reply-To: <01020158d1de0e71-ac079bb9-bc7d-4fb7-9ff7-60fd6955116b-000000@eu-west-1.amazonses.com>
On 6 December 2016 at 02:02, Nuno Subtil <subtil@gmail.com> wrote:
> Extends the submit command to support shelving a commit instead of
> submitting it to p4 (similar to --prepare-p4-only).
Is this just the same as these two changes?
http://www.spinics.net/lists/git/msg290755.html
http://www.spinics.net/lists/git/msg291103.html
Thanks,
Luke
>
> Signed-off-by: Nuno Subtil <subtil@gmail.com>
> ---
> git-p4.py | 36 ++++++++++++++++++++++++++++++------
> 1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52..3c4be22 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1286,6 +1286,8 @@ def __init__(self):
> optparse.make_option("--export-labels", dest="exportLabels", action="store_true"),
> optparse.make_option("--dry-run", "-n", dest="dry_run", action="store_true"),
> optparse.make_option("--prepare-p4-only", dest="prepare_p4_only", action="store_true"),
> + optparse.make_option("--shelve-only", dest="shelve_only", action="store_true", help="Create P4 shelf for first change that would be submitted (using a new CL)"),
> + optparse.make_option("--shelve-cl", dest="shelve_cl", help="Replace shelf under existing CL number (previously shelved files will be deleted)"),
> optparse.make_option("--conflict", dest="conflict_behavior",
> choices=self.conflict_behavior_choices),
> optparse.make_option("--branch", dest="branch"),
> @@ -1297,6 +1299,8 @@ def __init__(self):
> self.preserveUser = gitConfigBool("git-p4.preserveUser")
> self.dry_run = False
> self.prepare_p4_only = False
> + self.shelve_only = False
> + self.shelve_cl = None
> self.conflict_behavior = None
> self.isWindows = (platform.system() == "Windows")
> self.exportLabels = False
> @@ -1496,6 +1500,12 @@ def prepareSubmitTemplate(self):
> else:
> inFilesSection = False
> else:
> + if self.shelve_only and self.shelve_cl:
> + if line.startswith("Change:"):
> + line = "Change: %s\n" % self.shelve_cl
> + if line.startswith("Status:"):
> + line = "Status: pending\n"
> +
> if line.startswith("Files:"):
> inFilesSection = True
>
> @@ -1785,7 +1795,11 @@ def applyCommit(self, id):
> if self.isWindows:
> message = message.replace("\r\n", "\n")
> submitTemplate = message[:message.index(separatorLine)]
> - p4_write_pipe(['submit', '-i'], submitTemplate)
> +
> + if self.shelve_only:
> + p4_write_pipe(['shelve', '-i', '-r'], submitTemplate)
> + else:
> + p4_write_pipe(['submit', '-i'], submitTemplate)
>
> if self.preserveUser:
> if p4User:
> @@ -1799,12 +1813,17 @@ def applyCommit(self, id):
> # new file. This leaves it writable, which confuses p4.
> for f in pureRenameCopy:
> p4_sync(f, "-f")
> - submitted = True
> +
> + if not self.shelve_only:
> + submitted = True
>
> finally:
> # skip this patch
> if not submitted:
> - print "Submission cancelled, undoing p4 changes."
> + if not self.shelve_only:
> + print "Submission cancelled, undoing p4 changes."
> + else:
> + print "Change shelved, undoing p4 changes."
> for f in editedFiles:
> p4_revert(f)
> for f in filesToAdd:
> @@ -2034,9 +2053,13 @@ def run(self, args):
> if ok:
> applied.append(commit)
> else:
> - if self.prepare_p4_only and i < last:
> - print "Processing only the first commit due to option" \
> - " --prepare-p4-only"
> + if (self.prepare_p4_only or self.shelve_only) and i < last:
> + if self.prepare_p4_only:
> + print "Processing only the first commit due to option" \
> + " --prepare-p4-only"
> + else:
> + print "Processing only the first commit due to option" \
> + " --shelve-only"
> break
> if i < last:
> quit = False
> @@ -3638,6 +3661,7 @@ def printUsage(commands):
> "debug" : P4Debug,
> "submit" : P4Submit,
> "commit" : P4Submit,
> + "shelve" : P4Submit,
> "sync" : P4Sync,
> "rebase" : P4Rebase,
> "clone" : P4Clone,
>
> --
> https://github.com/git/git/pull/309
^ permalink raw reply
* Re: git 2.11.0 error when pushing to remote located on a windows share
From: Torsten Bögershausen @ 2016-12-06 3:09 UTC (permalink / raw)
To: thomas.attwood, peff; +Cc: git
In-Reply-To: <AABB04BF1441D24CB4E9FCF46394F17D666F3805@exchmbx01>
On 2016-12-05 12:05, thomas.attwood@stfc.ac.uk wrote:
> On Sun, 4 Dec 2016 08:09:14 +0000, Torsten Bögershausen wrote:
>> There seems to be another issue, which may or may not being related:
>> https://github.com/git-for-windows/git/issues/979
>
> I think this is the same issue. I've posted my trace command output there as
> It might be more appropriate:
> https://github.com/git-for-windows/git/issues/979#issuecomment-264816175
>
Thanks for the trace.
I think that the problem comes from the "cwd", when a UNC name is used.
cd //SERVER/share/somedir
does not work under Windows, the is no chance to change into that directory.
Does anybody know out of his head why and since when we change the directory
like this ?
Or "git bisect" may help.
^ permalink raw reply
* [PATCH] git-p4: add p4 shelf support
From: Nuno Subtil @ 2016-12-06 2:02 UTC (permalink / raw)
To: git
Extends the submit command to support shelving a commit instead of
submitting it to p4 (similar to --prepare-p4-only).
Signed-off-by: Nuno Subtil <subtil@gmail.com>
---
git-p4.py | 36 ++++++++++++++++++++++++++++++------
1 file changed, 30 insertions(+), 6 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index fd5ca52..3c4be22 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1286,6 +1286,8 @@ def __init__(self):
optparse.make_option("--export-labels", dest="exportLabels", action="store_true"),
optparse.make_option("--dry-run", "-n", dest="dry_run", action="store_true"),
optparse.make_option("--prepare-p4-only", dest="prepare_p4_only", action="store_true"),
+ optparse.make_option("--shelve-only", dest="shelve_only", action="store_true", help="Create P4 shelf for first change that would be submitted (using a new CL)"),
+ optparse.make_option("--shelve-cl", dest="shelve_cl", help="Replace shelf under existing CL number (previously shelved files will be deleted)"),
optparse.make_option("--conflict", dest="conflict_behavior",
choices=self.conflict_behavior_choices),
optparse.make_option("--branch", dest="branch"),
@@ -1297,6 +1299,8 @@ def __init__(self):
self.preserveUser = gitConfigBool("git-p4.preserveUser")
self.dry_run = False
self.prepare_p4_only = False
+ self.shelve_only = False
+ self.shelve_cl = None
self.conflict_behavior = None
self.isWindows = (platform.system() == "Windows")
self.exportLabels = False
@@ -1496,6 +1500,12 @@ def prepareSubmitTemplate(self):
else:
inFilesSection = False
else:
+ if self.shelve_only and self.shelve_cl:
+ if line.startswith("Change:"):
+ line = "Change: %s\n" % self.shelve_cl
+ if line.startswith("Status:"):
+ line = "Status: pending\n"
+
if line.startswith("Files:"):
inFilesSection = True
@@ -1785,7 +1795,11 @@ def applyCommit(self, id):
if self.isWindows:
message = message.replace("\r\n", "\n")
submitTemplate = message[:message.index(separatorLine)]
- p4_write_pipe(['submit', '-i'], submitTemplate)
+
+ if self.shelve_only:
+ p4_write_pipe(['shelve', '-i', '-r'], submitTemplate)
+ else:
+ p4_write_pipe(['submit', '-i'], submitTemplate)
if self.preserveUser:
if p4User:
@@ -1799,12 +1813,17 @@ def applyCommit(self, id):
# new file. This leaves it writable, which confuses p4.
for f in pureRenameCopy:
p4_sync(f, "-f")
- submitted = True
+
+ if not self.shelve_only:
+ submitted = True
finally:
# skip this patch
if not submitted:
- print "Submission cancelled, undoing p4 changes."
+ if not self.shelve_only:
+ print "Submission cancelled, undoing p4 changes."
+ else:
+ print "Change shelved, undoing p4 changes."
for f in editedFiles:
p4_revert(f)
for f in filesToAdd:
@@ -2034,9 +2053,13 @@ def run(self, args):
if ok:
applied.append(commit)
else:
- if self.prepare_p4_only and i < last:
- print "Processing only the first commit due to option" \
- " --prepare-p4-only"
+ if (self.prepare_p4_only or self.shelve_only) and i < last:
+ if self.prepare_p4_only:
+ print "Processing only the first commit due to option" \
+ " --prepare-p4-only"
+ else:
+ print "Processing only the first commit due to option" \
+ " --shelve-only"
break
if i < last:
quit = False
@@ -3638,6 +3661,7 @@ def printUsage(commands):
"debug" : P4Debug,
"submit" : P4Submit,
"commit" : P4Submit,
+ "shelve" : P4Submit,
"sync" : P4Sync,
"rebase" : P4Rebase,
"clone" : P4Clone,
--
https://github.com/git/git/pull/309
^ 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