* Re: regression in multi-threaded git-pack-index
From: Thomas Rast @ 2013-03-19 10:29 UTC (permalink / raw)
To: Jeff King
Cc: Thomas Rast, Stefan Zager, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319102422.GB6341@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Mar 19, 2013 at 06:08:00AM -0400, Jeff King wrote:
>
>> @@ -538,6 +539,8 @@ static void resolve_delta(struct object_entry *delta_obj,
>>
>> delta_obj->real_type = base->obj->real_type;
>> delta_obj->delta_depth = base->obj->delta_depth + 1;
>> + if (deepest_delta < delta_obj->delta_depth)
>> + deepest_delta = delta_obj->delta_depth;
>> delta_obj->base_object_no = base->obj - objects;
>> delta_data = get_data_from_pack(delta_obj);
>> base_data = get_base_data(base);
>>
>> and valgrind reports an uninitialized value in the conditional. But we
>> can see that deepest_delta is static, and therefore always has some
>> value. And delta_obj->delta_depth is set in the line above. So both
>> should have some known value, unless they are computed from unknown
>> values. In that case, shouldn't valgrind have previously noticed when we
>> accessed those unknown values?
>
> Ah, indeed. Putting:
>
> fprintf(stderr, "%lu\n", base->obj->delta_depth);
>
> before the conditional reveals that base->obj->delta_depth is
> uninitialized, which is the real problem. I'm sure there is some
> perfectly logical explanation for why valgrind can't detect its use
> during the assignment, but I'm not sure what it is.
That's simply because you would get far too much noise. It only reports
an uninitialized value when it actually gets used in a conditional or
for output (syscalls), which is when they matter.
You can use --track-origins=yes to see where the undefined value came
from, but it's veeeery slow.
> It looks like the delta_depth value was
> introduced in 38a4556 (index-pack: start learning to emulate
> "verify-pack -v", 2011-06-03), and it used only for showing the chain
> depths with --verify-stat. So it is almost certainly not related to
> Stefan's original problem, but it does mean we've probably been
> computing bogus chain lengths.
Nice catch!
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Jeff King @ 2013-03-19 10:24 UTC (permalink / raw)
To: Thomas Rast; +Cc: Stefan Zager, git, Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319100800.GA6341@sigill.intra.peff.net>
On Tue, Mar 19, 2013 at 06:08:00AM -0400, Jeff King wrote:
> @@ -538,6 +539,8 @@ static void resolve_delta(struct object_entry *delta_obj,
>
> delta_obj->real_type = base->obj->real_type;
> delta_obj->delta_depth = base->obj->delta_depth + 1;
> + if (deepest_delta < delta_obj->delta_depth)
> + deepest_delta = delta_obj->delta_depth;
> delta_obj->base_object_no = base->obj - objects;
> delta_data = get_data_from_pack(delta_obj);
> base_data = get_base_data(base);
>
> and valgrind reports an uninitialized value in the conditional. But we
> can see that deepest_delta is static, and therefore always has some
> value. And delta_obj->delta_depth is set in the line above. So both
> should have some known value, unless they are computed from unknown
> values. In that case, shouldn't valgrind have previously noticed when we
> accessed those unknown values?
Ah, indeed. Putting:
fprintf(stderr, "%lu\n", base->obj->delta_depth);
before the conditional reveals that base->obj->delta_depth is
uninitialized, which is the real problem. I'm sure there is some
perfectly logical explanation for why valgrind can't detect its use
during the assignment, but I'm not sure what it is.
At any rate, doing this:
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index ed4c3bb..73686af 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -538,6 +538,8 @@ static void resolve_delta(struct object_entry *delta_obj,
void *base_data, *delta_data;
delta_obj->real_type = base->obj->real_type;
+ if (!is_delta_type(base->obj->type))
+ base->obj->delta_depth = 0;
delta_obj->delta_depth = base->obj->delta_depth + 1;
if (deepest_delta < delta_obj->delta_depth)
deepest_delta = delta_obj->delta_depth;
makes the warning go away. It looks like the delta_depth value was
introduced in 38a4556 (index-pack: start learning to emulate
"verify-pack -v", 2011-06-03), and it used only for showing the chain
depths with --verify-stat. So it is almost certainly not related to
Stefan's original problem, but it does mean we've probably been
computing bogus chain lengths.
There may be a more reasonable place to set base->obj->delta_depth than
right here; I'll see if I can cook up a real patch.
-Peff
^ permalink raw reply related
* Re: regression in multi-threaded git-pack-index
From: Jeff King @ 2013-03-19 10:08 UTC (permalink / raw)
To: Thomas Rast; +Cc: Stefan Zager, git, Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319095943.GA6031@sigill.intra.peff.net>
On Tue, Mar 19, 2013 at 05:59:43AM -0400, Jeff King wrote:
> > Yes, that has been my experience with valgrind false positives, too. But
> > if this is a real problem, it may be different from the OP's issue. It
> > seems to trigger for me in v1.7.10, before Duy's threading patches. It
> > does not seem to be in v1.7.5. I'm bisecting now.
>
> Hmph. It bisects to Junio's d1a0ed1 (index-pack: show histogram when
> emulating "verify-pack -v", 2011-06-03), which introduces those lines.
> The deepest_delta variable is static, so by definition it is always
> initialized to something. So I guess some objects may not have
> delta_depth set. Still looking.
I'm doubly confused now. The commit in question introduces this:
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index aa3c9c6..ed4c3bb 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -70,6 +70,7 @@ static off_t consumed_bytes;
static unsigned char input_buffer[4096];
static unsigned int input_offset, input_len;
static off_t consumed_bytes;
+static unsigned deepest_delta;
static git_SHA_CTX input_ctx;
static uint32_t input_crc32;
static int input_fd, output_fd, pack_fd;
@@ -538,6 +539,8 @@ static void resolve_delta(struct object_entry *delta_obj,
delta_obj->real_type = base->obj->real_type;
delta_obj->delta_depth = base->obj->delta_depth + 1;
+ if (deepest_delta < delta_obj->delta_depth)
+ deepest_delta = delta_obj->delta_depth;
delta_obj->base_object_no = base->obj - objects;
delta_data = get_data_from_pack(delta_obj);
base_data = get_base_data(base);
and valgrind reports an uninitialized value in the conditional. But we
can see that deepest_delta is static, and therefore always has some
value. And delta_obj->delta_depth is set in the line above. So both
should have some known value, unless they are computed from unknown
values. In that case, shouldn't valgrind have previously noticed when we
accessed those unknown values?
-Peff
^ permalink raw reply related
* Re: [ITCH] Specify refspec without remote
From: Jeff King @ 2013-03-19 10:02 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <CALkWK0=WCsXErHft9RbDOeehon7E2oCj5-YT6Ph+8bLFW-5JaQ@mail.gmail.com>
On Tue, Mar 19, 2013 at 03:28:12PM +0530, Ramkumar Ramachandra wrote:
> > I think the ambiguity is a little more complex than that, because we
> > cannot enumerate the universe of all remotes. Keep in mind that we can
> > take either a configured remote or a URL (or ssh host). So what does:
> >
> > git push foo:bar
> >
> > mean? Is it pushing "refs/heads/foo" to "refs/heads/bar" on "origin"? Or
> > is it using the default refspecs to push to the "bar" repo on the host
> > "foo" over ssh?
>
> Wait, why does git-push support pushing to a URL directly? Shouldn't
> the user be required to create a new remote out of the URL and push to
> that? What happens to upstream branches if we directly push to a URL?
I do not recall the exact history, but I would not be surprised if git
first learned to push to a URL, and later learned about configured
remotes. I do not use it that often myself these days, but I find it
occasionally useful for one-off pushes (e.g., pushing a normally private
repo to a temporary publishing point to share with somebody else).
In that case, upstream branches are not touched at all (because we do
not have a configured remote whose fetch refspec we can examine).
-Peff
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Jeff King @ 2013-03-19 9:59 UTC (permalink / raw)
To: Thomas Rast; +Cc: Stefan Zager, git, Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319093034.GA29997@sigill.intra.peff.net>
On Tue, Mar 19, 2013 at 05:30:34AM -0400, Jeff King wrote:
> On Tue, Mar 19, 2013 at 09:17:32AM +0100, Thomas Rast wrote:
>
> > > but the line in question is:
> > >
> > > if (deepest_delta < delta_obj->delta_depth)
> > >
> > > And in the debugger, both of those variables appear to have sane values
> > > (nor should either impacted by the patch you bisected to). On top of
> > > that, running with pack.threads=1 produces the same error. So I think it
> > > may be a false positive from valgrind, and unrelated to your issue.
> >
> > I find that somewhat unlikely, for two reasons: memcheck is actually
> > quite good at finding uninitialized memory use, it just isn't that good
> > at distinguishing if it makes a difference. Most false positives are of
> > the "loading an entire word and discarding most of it" kind.
>
> Yes, that has been my experience with valgrind false positives, too. But
> if this is a real problem, it may be different from the OP's issue. It
> seems to trigger for me in v1.7.10, before Duy's threading patches. It
> does not seem to be in v1.7.5. I'm bisecting now.
Hmph. It bisects to Junio's d1a0ed1 (index-pack: show histogram when
emulating "verify-pack -v", 2011-06-03), which introduces those lines.
The deepest_delta variable is static, so by definition it is always
initialized to something. So I guess some objects may not have
delta_depth set. Still looking.
-Peff
^ permalink raw reply
* Re: [ITCH] Specify refspec without remote
From: Ramkumar Ramachandra @ 2013-03-19 9:58 UTC (permalink / raw)
To: Jeff King; +Cc: Git List
In-Reply-To: <20130318170804.GA15924@sigill.intra.peff.net>
Jeff King wrote:
> On Mon, Mar 18, 2013 at 10:28:59PM +0530, Ramkumar Ramachandra wrote:
>> Is there a reason for the remote not being optional, or are we just
>> waiting for a patch? The only problem I can foresee is very minor:
>> there is a ref with the same name as a remote; in this case, we'd have
>> to specify both the remote and the ref.
>
> I think the ambiguity is a little more complex than that, because we
> cannot enumerate the universe of all remotes. Keep in mind that we can
> take either a configured remote or a URL (or ssh host). So what does:
>
> git push foo:bar
>
> mean? Is it pushing "refs/heads/foo" to "refs/heads/bar" on "origin"? Or
> is it using the default refspecs to push to the "bar" repo on the host
> "foo" over ssh?
Wait, why does git-push support pushing to a URL directly? Shouldn't
the user be required to create a new remote out of the URL and push to
that? What happens to upstream branches if we directly push to a URL?
^ permalink raw reply
* [BUG?] rebase -i: edit versus unmerged changes
From: Ramkumar Ramachandra @ 2013-03-19 9:37 UTC (permalink / raw)
To: Git List
Hi,
I have the following instruction sheet:
pick 2f7c86e remote.c: simplify a bit of code using git_config_string()
pick c7f7ae4 remote.c: introduce a way to have different remotes for
fetch/push
edit 7038841 remote.c: introduce remote.pushdefault
edit 73413b0 remote.c: introduce branch.<name>.pushremote
pick 7f47687 WIP: fetch-push: write a test
When I get to 7038841, I edit config.txt, stage the changes and commit
--amend, before executing rebase --continue. When I get to 73413b0
(the next commit), I get a conflict which is auto-resolved by rerere.
So, I just make further changes to config.txt, and commit --amend
before executing rebase --continue as before. Unfortunately, this
squashes 73413b0 into 7038841, because the merge conflict requires a
commit to resolve the merge and pick the commit; this is different
from commit --amend, which worked in the previous case, because the
commit was already picked.
I know that this is expected behavior, but is there an easy way to get
rid of this inconsistency? I'd ideally like everything to use the
sequencer, and have uniform --continue/ --abort/ --skip semantics, but
that would require re-implementing all the shell commands in C unless
we can think of exposing a generic API using stdin or something. Does
anyone have ideas in this direction?
Thanks.
Ram
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Jeff King @ 2013-03-19 9:30 UTC (permalink / raw)
To: Thomas Rast; +Cc: Stefan Zager, git, Nguyễn Thái Ngọc Duy
In-Reply-To: <87fvzrajmr.fsf@pctrast.inf.ethz.ch>
On Tue, Mar 19, 2013 at 09:17:32AM +0100, Thomas Rast wrote:
> > but the line in question is:
> >
> > if (deepest_delta < delta_obj->delta_depth)
> >
> > And in the debugger, both of those variables appear to have sane values
> > (nor should either impacted by the patch you bisected to). On top of
> > that, running with pack.threads=1 produces the same error. So I think it
> > may be a false positive from valgrind, and unrelated to your issue.
>
> I find that somewhat unlikely, for two reasons: memcheck is actually
> quite good at finding uninitialized memory use, it just isn't that good
> at distinguishing if it makes a difference. Most false positives are of
> the "loading an entire word and discarding most of it" kind.
Yes, that has been my experience with valgrind false positives, too. But
if this is a real problem, it may be different from the OP's issue. It
seems to trigger for me in v1.7.10, before Duy's threading patches. It
does not seem to be in v1.7.5. I'm bisecting now.
-Peff
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Thomas Rast @ 2013-03-19 8:17 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Zager, git, Nguyễn Thái Ngọc Duy
In-Reply-To: <20130316114118.GA1940@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Mar 15, 2013 at 03:42:40PM -0700, Stefan Zager wrote:
>
>> We have uncovered a regression in this commit:
>>
>> b8a2486f1524947f232f657e9f2ebf44e3e7a243
>>
>> The symptom is that 'git fetch' dies with:
>>
>> error: index-pack died of signal 10
>> fatal: index-pack failed
>>
>> I have only been able to reproduce it on a Mac thus far; will try
>> ubuntu next. We can make it go away by running:
>>
>> git config pack.threads 1
>
> I couldn't reproduce the problem on Linux with the instructions you
> gave. I did try running it under valgrind and it produced:
>
> ==2380== Conditional jump or move depends on uninitialised value(s)
> ==2380== at 0x441631: resolve_delta (index-pack.c:837)
> ==2380== by 0x4419D6: find_unresolved_deltas_1 (index-pack.c:898)
> ==2380== by 0x441A45: find_unresolved_deltas (index-pack.c:914)
> ==2380== by 0x4427CA: fix_unresolved_deltas (index-pack.c:1232)
> ==2380== by 0x4421F5: conclude_pack (index-pack.c:1111)
> ==2380== by 0x443A5C: cmd_index_pack (index-pack.c:1604)
> ==2380== by 0x4058A2: run_builtin (git.c:281)
> ==2380== by 0x405A35: handle_internal_command (git.c:443)
> ==2380== by 0x405C01: main (git.c:532)
>
> but the line in question is:
>
> if (deepest_delta < delta_obj->delta_depth)
>
> And in the debugger, both of those variables appear to have sane values
> (nor should either impacted by the patch you bisected to). On top of
> that, running with pack.threads=1 produces the same error. So I think it
> may be a false positive from valgrind, and unrelated to your issue.
I find that somewhat unlikely, for two reasons: memcheck is actually
quite good at finding uninitialized memory use, it just isn't that good
at distinguishing if it makes a difference. Most false positives are of
the "loading an entire word and discarding most of it" kind.
Second, the thread-debugging valgrind tools (drd and helgrind) also
complain about exactly this line:
DRD says:
==20987== Thread 4:
==20987== Conflicting load by thread 4 at 0x007a70d0 size 4
==20987== at 0x43A783: resolve_delta (index-pack.c:837)
==20987== by 0x43A94F: find_unresolved_deltas (index-pack.c:898)
==20987== by 0x43B0F8: threaded_second_pass (index-pack.c:945)
==20987== by 0x4C2CF60: vgDrd_thread_wrapper (drd_pthread_intercepts.c:341)
==20987== by 0x542FE0D: start_thread (in /lib64/libpthread-2.15.so)
==20987== Allocation context: BSS section of /home/thomas/.local/bin/git
==20987== Other segment start (thread 2)
==20987== at 0x4C30A1F: pthread_mutex_unlock (drd_pthread_intercepts.c:665)
==20987== by 0x43B06E: threaded_second_pass (index-pack.c:122)
==20987== by 0x4C2CF60: vgDrd_thread_wrapper (drd_pthread_intercepts.c:341)
==20987== by 0x542FE0D: start_thread (in /lib64/libpthread-2.15.so)
==20987== Other segment end (thread 2)
==20987== at 0x5436AE3: ??? (in /lib64/libpthread-2.15.so)
==20987== by 0x439C76: unpack_data.constprop.8 (index-pack.c:528)
==20987== by 0x439EA7: get_base_data (index-pack.c:571)
==20987== by 0x43A7B4: resolve_delta (index-pack.c:841)
==20987== by 0x43A94F: find_unresolved_deltas (index-pack.c:898)
==20987== by 0x43B0F8: threaded_second_pass (index-pack.c:945)
==20987== by 0x4C2CF60: vgDrd_thread_wrapper (drd_pthread_intercepts.c:341)
==20987== by 0x542FE0D: start_thread (in /lib64/libpthread-2.15.so)
helgrind says:
==21160== Possible data race during read of size 4 at 0x7A70D0 by thread #3
==21160== Locks held: none
==21160== at 0x43A783: resolve_delta (index-pack.c:837)
==21160== by 0x43A94F: find_unresolved_deltas (index-pack.c:898)
==21160== by 0x43B0F8: threaded_second_pass (index-pack.c:945)
==21160== by 0x4C2D35D: mythread_wrapper (hg_intercepts.c:219)
==21160== by 0x5424E0D: start_thread (in /lib64/libpthread-2.15.so)
==21160==
==21160== This conflicts with a previous write of size 4 by thread #2
==21160== Locks held: none
==21160== at 0x43A78E: resolve_delta (index-pack.c:838)
==21160== by 0x43A94F: find_unresolved_deltas (index-pack.c:898)
==21160== by 0x43B0F8: threaded_second_pass (index-pack.c:945)
==21160== by 0x4C2D35D: mythread_wrapper (hg_intercepts.c:219)
==21160== by 0x5424E0D: start_thread (in /lib64/libpthread-2.15.so)
You were apparently just lucky in catching it before it was even
initialized.
I can reproduce the above warnings with
valgrind --tool=helgrind --trace-children=yes git index-pack <any_pack>
i.e., it does not seem to depend on the pack (sample size 3, packs
looked at were from my git.git).
Duy, what was the reasoning why resolve_delta() does not need to hold
locks when it looks when it looks at deepest_delta? My coffee levels
aren't up to this task yet. It certainly seems extremely dubious to me,
as the code uses the global deepest_delta in threaded sections. You can
probably argue that the load/store is atomic on most(?) platforms, but
you get no guarantees that deepest_delta at any time in fact holds the
maximum value of delta_obj->delta_depth.
Furthermore there's another warning shown by helgrind:
==21160== Possible data race during write of size 4 at 0x7A7060 by thread #2
==21160== Locks held: 1, at address 0x7A70E0
==21160== at 0x43A840: resolve_delta (index-pack.c:853)
==21160== by 0x43A94F: find_unresolved_deltas (index-pack.c:898)
==21160== by 0x43B0F8: threaded_second_pass (index-pack.c:945)
==21160== by 0x4C2D35D: mythread_wrapper (hg_intercepts.c:219)
==21160== by 0x5424E0D: start_thread (in /lib64/libpthread-2.15.so)
==21160==
==21160== This conflicts with a previous read of size 4 by thread #3
==21160== Locks held: 1, at address 0x7A7080
==21160== at 0x43AFC8: threaded_second_pass (index-pack.c:955)
==21160== by 0x4C2D35D: mythread_wrapper (hg_intercepts.c:219)
==21160== by 0x5424E0D: start_thread (in /lib64/libpthread-2.15.so)
That one really seems to be a false positive in the sense that
threaded_second_pass() doesn't really care if it gets a bad value for
nr_resolved_deltas. The only thing that matters is that the counter
increment is done with counter_mutex held.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: Path Too Long fix
From: steven Loker @ 2013-03-19 7:20 UTC (permalink / raw)
To: git
In-Reply-To: <3214ab99-ad65-4c97-839f-a6210e74e868@i39g2000prd.googlegroups.com>
Just fix it by downloading longPathTool. It works on my file since i dont
have time Copypasting,moving and renaming files.no hassle.
http://LongPathTool.com <http://LongPathTool.com>
--
View this message in context: http://git.661346.n2.nabble.com/Path-Too-Long-fix-tp6156555p7580026.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Matthieu Moy @ 2013-03-19 6:21 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, Junio C Hamano, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319034822.GL5062@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> The warning about use of 'git add -u' with no pathspec is annoying
> because it serves no purpose in this case. So suppress the warning
> unless there are changes outside the cwd that are not being added.
No time to review the code now. I thought about implementing something
like that, but did not do it because I didn't want the change in the
code to be too big. At some point, we'll have to remove the warning and
it's easier with my version than with yours. But the "damage" to the
code do not seem too big, so that's probably OK and will actually reduce
the pain for some users.
Discussions showed that many people were already using "git add -u"
without knowing whether it was full tree or not, so for these people the
change is somehow harmless anyway ;-).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Jonathan Nieder @ 2013-03-19 5:44 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, Jeff King, Matthieu Moy, git
In-Reply-To: <CACsJy8AjqwGVRRiQGMG0AN5qqtvxLk8FHKfUzCZB-7BykrHfjg@mail.gmail.com>
Duy Nguyen wrote:
> My concern is run full-tree diff can be expensive on large repos (one
> of the reasons the user may choose to work from within a
> subdirectory). We can exit as soon as we find a difference outside
> $prefix. But in case we find nothing, we need to diff the whole
> worktree.
Yes. This is an argument for the single-diff approach that Junio
suggested, since at least it's not significantly slower than the
future default behavior ("git add -u :/").
Sysadmins and others helping to train users will need to make sure
people working on large repos understand that they can use "git add -u ."
to avoid a speed penalty.
Thanks for looking it over,
Jonathan
^ permalink raw reply
* Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Duy Nguyen @ 2013-03-19 5:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Jeff King, Matthieu Moy, git
In-Reply-To: <7vli9kkoci.fsf@alter.siamese.dyndns.org>
On Tue, Mar 19, 2013 at 11:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I am wondering if we can special case the no-pathspec case to have
> add_files_to_cache() call underlying run_diff_files() without any
> pathspec, inspect the paths that are modified and/or deleted in the
> update_callback, add ones that are under the $prefix while noticing
> the ones outside as warning worthy events.
My concern is run full-tree diff can be expensive on large repos (one
of the reasons the user may choose to work from within a
subdirectory). We can exit as soon as we find a difference outside
$prefix. But in case we find nothing, we need to diff the whole
worktree.
--
Duy
^ permalink raw reply
* Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Jonathan Nieder @ 2013-03-19 5:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Matthieu Moy, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <7vli9kkoci.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> The implemenation appears to run an extra diff_files() in addition
> to the one we already have to run in order to implement the "add -u"
> to collect modified/deleted paths. Is that the best we can do?
At least the following should be squashed in to avoid wasted effort in
the easy case (cwd at top of worktree). Thanks for catching it.
diff --git i/builtin/add.c w/builtin/add.c
index f05ec1c1..8e4cdfae 100644
--- i/builtin/add.c
+++ w/builtin/add.c
@@ -483,7 +483,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
* "git add -u ." in the future. This warning prepares for
* that change.
*/
- if (implicit_dot)
+ if (prefix && implicit_dot)
diff_files_with_callback(prefix, NULL,
warn_if_outside_pathspec, pathspec);
^ permalink raw reply related
* Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Jonathan Nieder @ 2013-03-19 5:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Matthieu Moy, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <7vli9kkoci.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> The implemenation appears to run an extra diff_files() in addition
> to the one we already have to run in order to implement the "add -u"
> to collect modified/deleted paths. Is that the best we can do?
>
> I am wondering if we can special case the no-pathspec case to have
> add_files_to_cache() call underlying run_diff_files() without any
> pathspec, inspect the paths that are modified and/or deleted in the
> update_callback, add ones that are under the $prefix while noticing
> the ones outside as warning worthy events.
Yes, that can work, for example like this (replacing the patch you're
replying to).
-- >8 --
Subject: add -u: only show pathless 'add -u' warning when changes exist outside cwd
A common workflow in large projects is to chdir into a subdirectory of
interest and only do work there:
cd src
vi foo.c
make test
git add -u
git commit
The upcoming change to 'git add -u' behavior would not affect such a
workflow: when the only changes present are in the current directory,
'git add -u' will add all changes, and whether that happens via an
implicit "." or implicit ":/" parameter is an unimportant
implementation detail.
The warning about use of 'git add -u' with no pathspec is annoying
because it serves no purpose in this case. So suppress the warning
unless there are changes outside the cwd that are not being added.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/add.c | 41 +++++++++++++++++++++++++++++++++++++----
1 file changed, 37 insertions(+), 4 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index a424e69d..e3ed5d93 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,6 +26,7 @@ static int take_worktree_changes;
struct update_callback_data {
int flags;
int add_errors;
+ const char **implicit_dot;
};
static const char *option_with_implicit_dot;
@@ -94,10 +95,26 @@ static void update_callback(struct diff_queue_struct *q,
{
int i;
struct update_callback_data *data = cbdata;
+ const char **implicit_dot = data->implicit_dot;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
const char *path = p->one->path;
+
+ /*
+ * Check if "git add -A" or "git add -u" was run from a
+ * subdirectory with a modified file outside that directory,
+ * and warn if so.
+ *
+ * "git add -u" will behave like "git add -u :/" instead of
+ * "git add -u ." in the future. This warning prepares for
+ * that change.
+ */
+ if (implicit_dot &&
+ !match_pathspec(implicit_dot, path, strlen(path), 0, NULL)) {
+ warn_pathless_add();
+ continue;
+ }
switch (fix_unmerged_status(p, data)) {
default:
die(_("unexpected diff status %c"), p->status);
@@ -121,17 +138,30 @@ static void update_callback(struct diff_queue_struct *q,
}
}
+#define ADD_CACHE_IMPLICIT_DOT 32
int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
{
struct update_callback_data data;
struct rev_info rev;
+
+ data.flags = flags & ~ADD_CACHE_IMPLICIT_DOT;
+ data.add_errors = 0;
+ data.implicit_dot = NULL;
+ if (flags & ADD_CACHE_IMPLICIT_DOT) {
+ /*
+ * Check for modified files throughout the worktree so
+ * update_callback has a chance to warn about changes
+ * outside the cwd.
+ */
+ data.implicit_dot = pathspec;
+ pathspec = NULL;
+ }
+
init_revisions(&rev, prefix);
setup_revisions(0, NULL, &rev, NULL);
init_pathspec(&rev.prune_data, pathspec);
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = update_callback;
- data.flags = flags;
- data.add_errors = 0;
rev.diffopt.format_callback_data = &data;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
@@ -371,6 +401,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
+ int implicit_dot = 0;
git_config(add_config, NULL);
@@ -400,10 +431,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
}
if (option_with_implicit_dot && !argc) {
static const char *here[2] = { ".", NULL };
- if (prefix)
+ if (prefix && addremove)
warn_pathless_add();
argc = 1;
argv = here;
+ implicit_dot = 1;
}
add_new_files = !take_worktree_changes && !refresh_only;
@@ -416,7 +448,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
(intent_to_add ? ADD_CACHE_INTENT : 0) |
(ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
(!(addremove || take_worktree_changes)
- ? ADD_CACHE_IGNORE_REMOVAL : 0));
+ ? ADD_CACHE_IGNORE_REMOVAL : 0)) |
+ (implicit_dot ? ADD_CACHE_IMPLICIT_DOT : 0);
if (require_pathspec && argc == 0) {
fprintf(stderr, _("Nothing specified, nothing added.\n"));
--
1.8.2.rc3
^ permalink raw reply related
* Re: [PATCH 0/8] Improve git-status --ignored
From: Duy Nguyen @ 2013-03-19 5:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: Karsten Blees, Git List, Erik Faye-Lund, Ramkumar Ramachandra,
Robert Zeh, Antoine Pelisse, Adam Spiers
In-Reply-To: <7vsj3skp5b.fsf@alter.siamese.dyndns.org>
On Tue, Mar 19, 2013 at 11:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karsten Blees <karsten.blees@gmail.com> writes:
>
>> This patch series addresses several bugs and performance issues in
>> .gitignore processing that came up in the inotify discussion.
>
> Thanks.
>
> How does this interact with the nd/read-directory-recursive-optim
> topic that has been cooking for a while?
I think 8/8 is another version of nd/read-directory-recursive-optim
--
Duy
^ permalink raw reply
* Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Junio C Hamano @ 2013-03-19 4:25 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, Matthieu Moy, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319034822.GL5062@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> A common workflow in large projects is to chdir into a subdirectory of
> interest and only do work there:
>
> cd src
> vi foo.c
> make test
> git add -u
> git commit
>
> The upcoming change to 'git add -u' behavior would not affect such a
> workflow: when the only changes present are in the current directory,
> 'git add -u' will add all changes, and whether that happens via an
> implicit "." or implicit ":/" parameter is an unimportant
> implementation detail.
>
> The warning about use of 'git add -u' with no pathspec is annoying
> because it serves no purpose in this case. So suppress the warning
> unless there are changes outside the cwd that are not being added.
That is a logical extension of the reason why we do not emit
warnings when run at the top level. A user who has known about and
is very much accustomed to the "current directory only" behaviour
may run "git add -u/-A" always from the top in the current project
she happens to be working on until Git 2.0 happens, and will not get
any warnings. We are already robbing the chance to learn about and
prepare for the upcoming change from her. And this patch makes it
even more so. It does not make things fundamentally worse, but it
makes it more likely to hurt such a user.
The implemenation appears to run an extra diff_files() in addition
to the one we already have to run in order to implement the "add -u"
to collect modified/deleted paths. Is that the best we can do?
I am wondering if we can special case the no-pathspec case to have
add_files_to_cache() call underlying run_diff_files() without any
pathspec, inspect the paths that are modified and/or deleted in the
update_callback, add ones that are under the $prefix while noticing
the ones outside as warning worthy events.
^ permalink raw reply
* Re: [PATCH 0/4] make pathless 'add [-u|-A]' warning less noisy
From: Jeff King @ 2013-03-19 4:25 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Matthieu Moy, Junio C Hamano, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319034415.GI5062@elie.Belkin>
On Mon, Mar 18, 2013 at 08:44:15PM -0700, Jonathan Nieder wrote:
> > The
> > config option added by this patch gives them such an option.
>
> I suspect the need for this config option is a sign that the warning
> is too eager. After all, the whole idea of the change being safe is
> that it shouldn't make a difference the way people usually use git,
> no?
>
> In other words, how about the following patches? With them applied,
> hopefully no one would mind even if the warning becomes a fatal error.
Clever. I think it would help in my case. I sometimes follow the
workflow you describe in patch 3 (i.e., just working in a subdir), and
sometimes do something more like:
$ vi foo.c
$ cd t
$ vi tXXXX-foo.sh
$ ./tXXXX-foo.sh
$ git add -u
With your patches, we would continue to warn about the second case, but
I think that is a good thing; git is not doing what I want. But by
reducing the false positives from the first case, I would start to
actually pay attention to the warning more.
> Jonathan Nieder (4):
> add: make pathless 'add [-u|-A]' warning a file-global function
> add: make warn_pathless_add() a no-op after first call
> add -u: only show pathless 'add -u' warning when changes exist outside cwd
> add -A: only show pathless 'add -A' warning when changes exist outside cwd
I don't see anything obviously wrong with the patches themselves. I
wonder if we would want to change the warning to be more explicit that
yes, there really were files that were impacted by this. And possibly
even list them.
I suspect I would not even mind that becoming the final behavior. I.e.,
going to:
$ cd subdir && git add -u
warning: Using 'git add -u' without a pathspec operates only on the
current subdirectory. Updates from the following files were NOT
staged:
file1
file2
other-subdir/file3
now, and then eventually converting the warning into a fatal error (and
demanding that the user use ":/" or "." as appropriate).
But in the long run, I guess defaulting to ":/" will be more convenient,
so there is no point in complaining about the ambiguity forever. And in
that case, since the warning is just a placeholder, I don't know that
it's worth much effort to make it fancier.
-Peff
^ permalink raw reply
* Re: [PATCH 0/8] Improve git-status --ignored
From: Junio C Hamano @ 2013-03-19 4:08 UTC (permalink / raw)
To: Karsten Blees
Cc: Git List, Erik Faye-Lund, Ramkumar Ramachandra, Robert Zeh,
Duy Nguyen, Antoine Pelisse, Adam Spiers
In-Reply-To: <514778E4.1040607@gmail.com>
Karsten Blees <karsten.blees@gmail.com> writes:
> This patch series addresses several bugs and performance issues in
> .gitignore processing that came up in the inotify discussion.
Thanks.
How does this interact with the nd/read-directory-recursive-optim
topic that has been cooking for a while?
^ permalink raw reply
* [PATCH 4/4] add -A: only show pathless 'add -A' warning when changes exist outside cwd
From: Jonathan Nieder @ 2013-03-19 3:49 UTC (permalink / raw)
To: Jeff King
Cc: Matthieu Moy, Junio C Hamano, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319034415.GI5062@elie.Belkin>
In the spirit of the recent similar change for 'git add -u', avoid
pestering users that restrict their attention to a subdirectory and
will not be affected by the coming change in the behavior of pathless
'git add -A'.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/add.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index f05ec1c1..56ac4519 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -160,7 +160,9 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
return !!data.add_errors;
}
-static char *prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
+#define WARN_IMPLICIT_DOT (1u << 0)
+static char *prune_directory(struct dir_struct *dir, const char **pathspec,
+ int prefix, unsigned flag)
{
char *seen;
int i, specs;
@@ -177,6 +179,16 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int
if (match_pathspec(pathspec, entry->name, entry->len,
prefix, seen))
*dst++ = entry;
+ else if (flag & WARN_IMPLICIT_DOT)
+ /*
+ * "git add -A" was run from a subdirectory with a
+ * new file outside that directory.
+ *
+ * "git add -A" will behave like "git add -A :/"
+ * instead of "git add -A ." in the future.
+ * Warn about the coming behavior change.
+ */
+ warn_pathless_add();
}
dir->nr = dst - dir->entries;
add_pathspec_matches_against_index(pathspec, seen, specs);
@@ -423,8 +435,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
}
if (option_with_implicit_dot && !argc) {
static const char *here[2] = { ".", NULL };
- if (prefix && addremove)
- warn_pathless_add();
argc = 1;
argv = here;
implicit_dot = 1;
@@ -464,9 +474,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
}
/* This picks up the paths that are not tracked */
- baselen = fill_directory(&dir, pathspec);
+ baselen = fill_directory(&dir, implicit_dot ? NULL : pathspec);
if (pathspec)
- seen = prune_directory(&dir, pathspec, baselen);
+ seen = prune_directory(&dir, pathspec, baselen,
+ implicit_dot ? WARN_IMPLICIT_DOT : 0);
}
if (refresh_only) {
--
1.8.2.rc3
^ permalink raw reply related
* [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Jonathan Nieder @ 2013-03-19 3:48 UTC (permalink / raw)
To: Jeff King
Cc: Matthieu Moy, Junio C Hamano, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319034415.GI5062@elie.Belkin>
A common workflow in large projects is to chdir into a subdirectory of
interest and only do work there:
cd src
vi foo.c
make test
git add -u
git commit
The upcoming change to 'git add -u' behavior would not affect such a
workflow: when the only changes present are in the current directory,
'git add -u' will add all changes, and whether that happens via an
implicit "." or implicit ":/" parameter is an unimportant
implementation detail.
The warning about use of 'git add -u' with no pathspec is annoying
because it serves no purpose in this case. So suppress the warning
unless there are changes outside the cwd that are not being added.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/add.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 44 insertions(+), 7 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index a424e69d..f05ec1c1 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -89,6 +89,22 @@ static int fix_unmerged_status(struct diff_filepair *p,
return DIFF_STATUS_MODIFIED;
}
+static void warn_if_outside_pathspec(struct diff_queue_struct *q,
+ struct diff_options *opt, void *cbdata)
+{
+ int i;
+ const char **pathspec = cbdata;
+
+ for (i = 0; i < q->nr; i++) {
+ const char *path = q->queue[i]->one->path;
+
+ if (!match_pathspec(pathspec, path, strlen(path), 0, NULL)) {
+ warn_pathless_add();
+ return;
+ }
+ }
+}
+
static void update_callback(struct diff_queue_struct *q,
struct diff_options *opt, void *cbdata)
{
@@ -121,20 +137,26 @@ static void update_callback(struct diff_queue_struct *q,
}
}
-int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
+static void diff_files_with_callback(const char *prefix, const char **pathspec,
+ diff_format_fn_t callback, void *data)
{
- struct update_callback_data data;
struct rev_info rev;
init_revisions(&rev, prefix);
setup_revisions(0, NULL, &rev, NULL);
init_pathspec(&rev.prune_data, pathspec);
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
- rev.diffopt.format_callback = update_callback;
- data.flags = flags;
- data.add_errors = 0;
- rev.diffopt.format_callback_data = &data;
+ rev.diffopt.format_callback = callback;
+ rev.diffopt.format_callback_data = data;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
+}
+
+int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
+{
+ struct update_callback_data data;
+ data.flags = flags;
+ data.add_errors = 0;
+ diff_files_with_callback(prefix, pathspec, update_callback, &data);
return !!data.add_errors;
}
@@ -371,6 +393,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
+ int implicit_dot = 0;
git_config(add_config, NULL);
@@ -400,10 +423,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
}
if (option_with_implicit_dot && !argc) {
static const char *here[2] = { ".", NULL };
- if (prefix)
+ if (prefix && addremove)
warn_pathless_add();
argc = 1;
argv = here;
+ implicit_dot = 1;
}
add_new_files = !take_worktree_changes && !refresh_only;
@@ -450,6 +474,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)
goto finish;
}
+ /*
+ * Check if "git add -A" or "git add -u" was run from a
+ * subdirectory with a modified file outside that directory,
+ * and warn if so.
+ *
+ * "git add -u" will behave like "git add -u :/" instead of
+ * "git add -u ." in the future. This warning prepares for
+ * that change.
+ */
+ if (implicit_dot)
+ diff_files_with_callback(prefix, NULL,
+ warn_if_outside_pathspec, pathspec);
+
if (pathspec) {
int i;
struct path_exclude_check check;
--
1.8.2.rc3
^ permalink raw reply related
* [PATCH 2/4] add: make warn_pathless_add() a no-op after first call
From: Jonathan Nieder @ 2013-03-19 3:46 UTC (permalink / raw)
To: Jeff King
Cc: Matthieu Moy, Junio C Hamano, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319034415.GI5062@elie.Belkin>
Make warn_pathless_add() print its warning the first time it is called
and do nothing if called again. This will make it easier to show the
warning on the fly when a relevant condition is detected without
risking showing it multiple times when multiple such conditions hold.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/add.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/builtin/add.c b/builtin/add.c
index a4028eea..a424e69d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -33,8 +33,13 @@ static const char *short_option_with_implicit_dot;
static void warn_pathless_add(void)
{
+ static int shown;
assert(option_with_implicit_dot && short_option_with_implicit_dot);
+ if (shown)
+ return;
+ shown = 1;
+
/*
* To be consistent with "git add -p" and most Git
* commands, we should default to being tree-wide, but
--
1.8.2.rc3
^ permalink raw reply related
* [PATCH 1/4] add: make pathless 'add [-u|-A]' warning a file-global function
From: Jonathan Nieder @ 2013-03-19 3:45 UTC (permalink / raw)
To: Jeff King
Cc: Matthieu Moy, Junio C Hamano, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319034415.GI5062@elie.Belkin>
Currently warn_pathless_add() is only called directly by cmd_add(),
but that is about to change. Move its definition higher in the file
and pass the "--update" or "--all" option name used in its message
through globals instead of function arguments to make it easier to
call without passing values that will not change through the call
chain.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/add.c | 69 +++++++++++++++++++++++++++++++----------------------------
1 file changed, 36 insertions(+), 33 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8f..a4028eea 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -28,6 +28,41 @@ struct update_callback_data {
int add_errors;
};
+static const char *option_with_implicit_dot;
+static const char *short_option_with_implicit_dot;
+
+static void warn_pathless_add(void)
+{
+ assert(option_with_implicit_dot && short_option_with_implicit_dot);
+
+ /*
+ * To be consistent with "git add -p" and most Git
+ * commands, we should default to being tree-wide, but
+ * this is not the original behavior and can't be
+ * changed until users trained themselves not to type
+ * "git add -u" or "git add -A". For now, we warn and
+ * keep the old behavior. Later, the behavior can be changed
+ * to tree-wide, keeping the warning for a while, and
+ * eventually we can drop the warning.
+ */
+ warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
+ "subdirectory of the tree will change in Git 2.0 and should not be used anymore.\n"
+ "To add content for the whole tree, run:\n"
+ "\n"
+ " git add %s :/\n"
+ " (or git add %s :/)\n"
+ "\n"
+ "To restrict the command to the current directory, run:\n"
+ "\n"
+ " git add %s .\n"
+ " (or git add %s .)\n"
+ "\n"
+ "With the current Git version, the command is restricted to the current directory."),
+ option_with_implicit_dot, short_option_with_implicit_dot,
+ option_with_implicit_dot, short_option_with_implicit_dot,
+ option_with_implicit_dot, short_option_with_implicit_dot);
+}
+
static int fix_unmerged_status(struct diff_filepair *p,
struct update_callback_data *data)
{
@@ -321,35 +356,6 @@ static int add_files(struct dir_struct *dir, int flags)
return exit_status;
}
-static void warn_pathless_add(const char *option_name, const char *short_name) {
- /*
- * To be consistent with "git add -p" and most Git
- * commands, we should default to being tree-wide, but
- * this is not the original behavior and can't be
- * changed until users trained themselves not to type
- * "git add -u" or "git add -A". For now, we warn and
- * keep the old behavior. Later, the behavior can be changed
- * to tree-wide, keeping the warning for a while, and
- * eventually we can drop the warning.
- */
- warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
- "subdirectory of the tree will change in Git 2.0 and should not be used anymore.\n"
- "To add content for the whole tree, run:\n"
- "\n"
- " git add %s :/\n"
- " (or git add %s :/)\n"
- "\n"
- "To restrict the command to the current directory, run:\n"
- "\n"
- " git add %s .\n"
- " (or git add %s .)\n"
- "\n"
- "With the current Git version, the command is restricted to the current directory."),
- option_name, short_name,
- option_name, short_name,
- option_name, short_name);
-}
-
int cmd_add(int argc, const char **argv, const char *prefix)
{
int exit_status = 0;
@@ -360,8 +366,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
- const char *option_with_implicit_dot = NULL;
- const char *short_option_with_implicit_dot = NULL;
git_config(add_config, NULL);
@@ -392,8 +396,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (option_with_implicit_dot && !argc) {
static const char *here[2] = { ".", NULL };
if (prefix)
- warn_pathless_add(option_with_implicit_dot,
- short_option_with_implicit_dot);
+ warn_pathless_add();
argc = 1;
argv = here;
}
--
1.8.2.rc3
^ permalink raw reply related
* [PATCH 0/4] make pathless 'add [-u|-A]' warning less noisy
From: Jonathan Nieder @ 2013-03-19 3:44 UTC (permalink / raw)
To: Jeff King
Cc: Matthieu Moy, Junio C Hamano, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130313041037.GB5378@sigill.intra.peff.net>
Hi,
Jeff King wrote:
> The
> config option added by this patch gives them such an option.
I suspect the need for this config option is a sign that the warning
is too eager. After all, the whole idea of the change being safe is
that it shouldn't make a difference the way people usually use git,
no?
In other words, how about the following patches? With them applied,
hopefully no one would mind even if the warning becomes a fatal error.
Looking forward to your thoughts,
Jonathan Nieder (4):
add: make pathless 'add [-u|-A]' warning a file-global function
add: make warn_pathless_add() a no-op after first call
add -u: only show pathless 'add -u' warning when changes exist outside cwd
add -A: only show pathless 'add -A' warning when changes exist outside cwd
builtin/add.c | 142 ++++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 99 insertions(+), 43 deletions(-)
^ permalink raw reply
* Re: building git ; need suggestion
From: David Aguilar @ 2013-03-19 2:11 UTC (permalink / raw)
To: Joydeep Bakshi; +Cc: Magnus Bäck, Fredrik Gustafsson, git
In-Reply-To: <9E0367AC-617A-440B-925E-5796CF2E1ADF@infoservices.in>
On Mon, Mar 18, 2013 at 5:24 AM, Joydeep Bakshi
<joydeep.bakshi@infoservices.in> wrote:
> I'm closer to my requirement. I have found gitweb simply provide a GUI for history check
> and code comparison. And the git itself is good enough to do the ACL stuff with hooks.
>
> I already have the following code to deploy the push into its work-tree
You should try gitolite. It has very flexible rules,
and it's already been implemented for you ;-)
https://github.com/sitaramc/gitolite
> ===========================
> #!/bin/bash
>
> while read oldrev newrev ref
> do
> branch=`echo $ref | cut -d/ -f3`
>
> if [ "master" == "$branch" ]; then
> git --work-tree=/path/under/root/dir/live-site/ checkout -f $branch
> echo 'Changes pushed live.'
> fi
>
> if [ "dev" == "$branch" ]; then
> git --work-tree=/path/under/root/dir/dev-site/ checkout -f $branch
> echo 'Changes pushed to dev.'
> fi
> done
> =========================
>
> This code can be extended for as many branches as you have.
>
> I now need a mechanism to restrict the user to it's own branch so that user can't push into
> any other branch in mistake.
>
> Say I have
>
> master branch -> only admin user can push here.
> dev branch -> only user dev1 , dev2 and master can push here.
> testing branch -> only user test1 and test2 can push here.
>
> I think this can also be done with pre-receive hook. Any suggestion on the hook design is
> welcome. Also this can be implemented on the above hook or in a separate hook.
> A separate hook is better due to maintainability and then I need to call multiple
> pre-receive hook. Please suggest.
>
> Thanks
>
>
>
> On 18-Mar-2013, at 11:14 AM, Joydeep Bakshi <joydeep.bakshi@infoservices.in> wrote:
>
>>
>> On 15-Mar-2013, at 6:44 PM, Magnus Bäck <baeck@google.com> wrote:
>>>>
>>>
>>> Right, but that's R/W permissions. Almost any piece of Git hosting
>>> software supports restriction of pushes. Discriminating *read* access
>>> between developers and maintenance people sounds like a disaster if it's
>>> the same organisation.
>>
>> Just restriction on push access is what required.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
David
^ 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