* [PATCHv2 1/2] push: change submodule default to check when submodules exist
From: Stefan Beller @ 2016-10-04 19:29 UTC (permalink / raw)
To: gitster; +Cc: git, peff, hvoigt, torvalds, Stefan Beller
In-Reply-To: <20161004182801.j3fdpewybatmibpo@sigill.intra.peff.net>
When working with submodules, it is easy to forget to push the submodules.
The setting 'check', which checks if any existing submodule is present on
at least one remote of the submodule remotes, is designed to prevent this
mistake.
Flipping the default to check for submodules is safer than the current
default of ignoring submodules while pushing.
However checking for submodules requires additional work[1], which annoys
users that do not use submodules, so we turn on the check for submodules
based on a cheap heuristic, the existence of the .git/modules directory.
That directory doesn't exist when no submodules are used and is only
created and populated when submodules are cloned/added.
When the submodule directory doesn't exist, a user may have changed the
gitlinks via plumbing commands. Currently the default is to not check.
RECURSE_SUBMODULES_DEFAULT is effectively RECURSE_SUBMODULES_OFF currently,
though it may change in the future. When no submodules exist such a check
is pointless as it would fail anyway, so let's just turn it off.
[1] https://public-inbox.org/git/CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com/
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Jeff wrote:
> Consulting .git/config is fine, I think. It's not like we don't read it
> (sometimes multiple times!) during the normal course of the program
> anyway. It's just a question of whether it makes more sense for the
> heuristic to kick in after "init", or only after "update". I don't know
> enough to have an opinion.
I think there is no difference in practice, however the "after update"
is way easier to implement and hence more maintainable (one lstat instead of
fiddeling with the config; that can go wrong easily).
Thanks,
Stefan
builtin/push.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/builtin/push.c b/builtin/push.c
index 3bb9d6b..06fd3bd 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -3,6 +3,7 @@
*/
#include "cache.h"
#include "refs.h"
+#include "dir.h"
#include "run-command.h"
#include "builtin.h"
#include "remote.h"
@@ -22,7 +23,7 @@ static int deleterefs;
static const char *receivepack;
static int verbosity;
static int progress = -1;
-static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+static int recurse_submodules;
static enum transport_family family;
static struct push_cas_option cas;
@@ -31,6 +32,19 @@ static const char **refspec;
static int refspec_nr;
static int refspec_alloc;
+static void preset_submodule_default(void)
+{
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addf(&sb, "%s/modules", get_git_dir());
+
+ if (file_exists(sb.buf))
+ recurse_submodules = RECURSE_SUBMODULES_CHECK;
+ else
+ recurse_submodules = RECURSE_SUBMODULES_OFF;
+
+ strbuf_release(&sb);
+}
+
static void add_refspec(const char *ref)
{
refspec_nr++;
@@ -552,6 +566,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
};
packet_trace_identity("push");
+ preset_submodule_default();
git_config(git_push_config, &flags);
argc = parse_options(argc, argv, prefix, options, push_usage, 0);
set_push_cert_flags(&flags, push_cert);
--
2.10.0.129.g35f6318
^ permalink raw reply related
* [PATCH 2/2] builtin/push: move flags do_push
From: Stefan Beller @ 2016-10-04 19:29 UTC (permalink / raw)
To: gitster; +Cc: git, peff, hvoigt, torvalds, Stefan Beller
In-Reply-To: <20161004192910.30649-1-sbeller@google.com>
In do_push we set the flags for other options, so let's make the code
more consistent and also apply the flags for recursing into submodules
there.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
No functional change intended, just a cleanup while we're at it.
Feel free to drop if it's too much churn.
builtin/push.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 06fd3bd..6690301 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -388,6 +388,11 @@ static int do_push(const char *repo, int flags,
" git push <name>\n"));
}
+ if (recurse_submodules == RECURSE_SUBMODULES_CHECK)
+ flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
+ else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)
+ flags |= TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND;
+
if (remote->mirror)
flags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);
@@ -576,11 +581,6 @@ int cmd_push(int argc, const char **argv, const char *prefix)
if (deleterefs && argc < 2)
die(_("--delete doesn't make sense without any refs"));
- if (recurse_submodules == RECURSE_SUBMODULES_CHECK)
- flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
- else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)
- flags |= TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND;
-
if (tags)
add_refspec("refs/tags/*");
--
2.10.0.129.g35f6318
^ permalink raw reply related
* Re: [PATCH v8 00/11] Git filter protocol
From: Jakub Narębski @ 2016-10-04 19:04 UTC (permalink / raw)
To: Lars Schneider
Cc: Junio C Hamano, Torsten Bögershausen, git, Jeff King,
Stefan Beller, Martin-Louis Bright, Ramsay Jones
In-Reply-To: <E9946E9F-6EE5-492B-B122-9078CEB88044@gmail.com>
W dniu 03.10.2016 o 19:13, Lars Schneider pisze:
>> On 01 Oct 2016, at 22:48, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 01.10.2016 o 20:59, Lars Schneider pisze:
>>> On 29 Sep 2016, at 23:27, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Lars Schneider <larsxschneider@gmail.com> writes:
>>>>
>>>> If the filter process refuses to die forever when Git told it to
>>>> shutdown (by closing the pipe to it, for example), that filter
>>>> process is simply buggy. I think we want users to become aware of
>>>> that, instead of Git leaving it behind, which essentially is to
>>>> sweep the problem under the rug.
>>
>> Well, it would be good to tell users _why_ Git is hanging, see below.
>
> Agreed. Do you think it is OK to write the message to stderr?
On the other hand, this is why GIT_TRACE (and GIT_TRACE_PERFORMANCE)
was invented for. We do not signal troubles with single-shot filters,
so I guess doing it for multi-file filters is not needed.
>>>> I agree with what Peff said elsewhere in the thread; if a filter
>>>> process wants to take time to clean things up while letting Git
>>>> proceed, it can do its own process management, but I think it is
>>>> sensible for Git to wait the filter process it directly spawned.
>>>
>>> To realize the approach above I prototyped the run-command patch below:
>>>
>>> I added an "exit_timeout" variable to the "child_process" struct.
>>> On exit, Git will close the pipe to the process and wait "exit_timeout"
>>> seconds until it kills the child process. If "exit_timeout" is negative
>>> then Git will wait until the process is done.
>>
>> That might be good approach. Probably the default would be to wait.
>
> I think I would prefer a 2sec timeout or something as default. This way
> we can ensure Git would not wait indefinitely for a buggy filter by default.
Actually this waiting for multi-file filter is only about waiting for
the shutdown process of the filter. The filter could still hang during
processing a file, and git would hang too, if I understand it correctly.
[...]
>> Also, how would one set default value of timeout for all process
>> based filters?
>
> I think we don't need that because a timeout is always specific
> to a filter (if the 2sec default is not sufficient).
All right (assuming that timeouts are good idea).
>>>
>>> + while ((waitpid(p->pid, &status, 0)) < 0 && errno == EINTR)
>>> + ; /* nothing */
>>
>> Ah, this loop is here because waiting on waitpid() can be interrupted
>> by the delivery of a signal to the calling process; though the result
>> is -1, not just any < 0.
>
> "< 0" is also used in wait_or_whine()
O.K. (though it doesn't necessary mean that it is correct, there
is another point for using "< 0").
[...]
>> There is also another complication: there can be more than one
>> long-running filter driver used. With this implementation we
>> wait for each of one in sequence, e.g. 10s + 10s + 10s.
>
> Good idea, I fixed that in the version below!
>
[...]
> [...] this function is also used with the async struct...
Hmmm... now I wonder if it is a good idea (similar treatment for
single-file async-invoked filter, and multi-file pkt-line filters).
For single-file one-shot filter (correct me if I am wrong):
- git sends contents to filter, signals end with EOF
(after process is started)
- in an async process:
- process is started
- git reads contents from filter, until EOF
- if process did not end, it is killed
For multi-process pkt-line based filter (simplified):
- process is started
- handshake
- for each file
- file is send to filter process over pkt-line,
end signalled with flush packet
- git reads from filter from pkt-line, until flush
- ...
See how single-shot filter is sent EOF, though in different part
of code. We need to signal multi-file filter that no more files
will be coming. Simplest solution is to send EOF (we could send
"command=shutdown" for example...) to filter, and wait for EOF
from filter (or for "status=finished" and EOF).
We could kill multi-file filter after sending last file and
receiving full response... but I think single-shot filter gets
killed only because it allows for very simple filters, and reusing
existing commands as filters.
[...]
> diff --git a/run-command.c b/run-command.c
> index 3269362..ca0feef 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -21,6 +21,9 @@ void child_process_clear(struct child_process *child)
>
> struct child_to_clean {
> pid_t pid;
> + char *name;
I guess it is here for output purposes?
Should we store full command here, or just name of <driver>?
> + int stdin;
I guess the name `stdin` for file _descriptor_ is something
used in other parts of convert.c code, isn't it?
> + int timeout;
Hmmm... we assume that timeout is in seconds, not millis or other
value, isn't it. timeout_sec would perhaps be unnecessarily long.
> struct child_to_clean *next;
> };
> static struct child_to_clean *children_to_clean;
> @@ -28,12 +31,53 @@ static int installed_child_cleanup_handler;
>
> static void cleanup_children(int sig, int in_signal)
> {
> + int status;
> + struct timeval tv;
> + time_t secs;
> + struct child_to_clean *p = children_to_clean;
> +
> + // Send EOF to children as indicator that Git will exit soon
> + while (p) {
> + if (p->timeout != 0) {
Here we use timeout == 0 as a special case, a special indicator
(IIUC for the single-shot filter case, where it is closed already).
This is not documented. Somebody setting timeout to "0" would
be surprised, isn't it?
> + if (p->stdin > 0)
> + close(p->stdin);
> + }
> + p = p->next;
> + }
> +
> while (children_to_clean) {
> - struct child_to_clean *p = children_to_clean;
> + p = children_to_clean;
> children_to_clean = p->next;
> +
> + if (p->timeout != 0) {
> + fprintf(stderr, _("Waiting for '%s' to finish..."), p->name);
> + if (p->timeout < 0) {
> + // No timeout given - wait indefinitely
> + while ((waitpid(p->pid, &status, 0)) < 0 && errno == EINTR)
> + ; /* nothing */
> + } else {
> + // Wait until timeout
> + gettimeofday(&tv, NULL);
> + secs = tv.tv_sec;
> + while (!waitpid(p->pid, &status, WNOHANG) &&
> + tv.tv_sec - secs < p->timeout) {
> + fprintf(stderr, _(" \rWaiting %lds for '%s' to finish..."),
> + p->timeout - tv.tv_sec + secs - 1, p->name);
> + gettimeofday(&tv, NULL);
> + sleep_millisec(10);
> + }
> + }
I wonder if we have some progress-printing code we can borrow
from, or just plain use (like progress report for long checkout).
> + if (waitpid(p->pid, &status, WNOHANG))
> + fprintf(stderr, _("done!\n"));
> + else
> + fprintf(stderr, _("timeout. Killing...\n"));
> + }
> +
> kill(p->pid, sig);
> - if (!in_signal)
> + if (!in_signal) {
> + free(p->name);
> free(p);
> + }
> }
> }
>
> @@ -49,10 +93,18 @@ static void cleanup_children_on_exit(void)
> cleanup_children(SIGTERM, 0);
> }
>
> -static void mark_child_for_cleanup(pid_t pid)
> +static void mark_child_for_cleanup_with_timeout(pid_t pid, const char *name, int stdin, int timeout)
> {
> struct child_to_clean *p = xmalloc(sizeof(*p));
> p->pid = pid;
> + p->timeout = timeout;
> + p->stdin = stdin;
> + if (name) {
> + p->name = xmalloc(strlen(name) + 1);
> + strcpy(p->name, name);
Don't we have xstrdup() for that, or am I mistaken?
> + } else {
> + p->name = "process";
Hmmmm...
> + }
> p->next = children_to_clean;
> children_to_clean = p;
>
> @@ -63,6 +115,13 @@ static void mark_child_for_cleanup(pid_t pid)
> }
> }
>
> +#ifdef NO_PTHREADS
> +static void mark_child_for_cleanup(pid_t pid, const char *name, int timeout, int stdin)
> +{
> + mark_child_for_cleanup_with_timeout(pid, NULL, 0, 0);
> +}
> +#endif
Uh?
> +
> static void clear_child_for_cleanup(pid_t pid)
> {
> struct child_to_clean **pp;
> @@ -422,7 +481,8 @@ int start_command(struct child_process *cmd)
> if (cmd->pid < 0)
> error_errno("cannot fork() for %s", cmd->argv[0]);
> else if (cmd->clean_on_exit)
> - mark_child_for_cleanup(cmd->pid);
> + mark_child_for_cleanup_with_timeout(
> + cmd->pid, cmd->argv[0], cmd->in, cmd->clean_on_exit_timeout);
All right, nice abstraction.
>
> /*
> * Wait for child's execvp. If the execvp succeeds (or if fork()
> @@ -483,7 +543,8 @@ int start_command(struct child_process *cmd)
> if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
> error_errno("cannot spawn %s", cmd->argv[0]);
> if (cmd->clean_on_exit && cmd->pid >= 0)
> - mark_child_for_cleanup(cmd->pid);
> + mark_child_for_cleanup_with_timeout(
> + cmd->pid, cmd->argv[0], cmd->in, cmd->clean_on_exit_timeout);
>
> argv_array_clear(&nargv);
> cmd->argv = sargv;
> diff --git a/run-command.h b/run-command.h
> index cf29a31..4c1c1f4 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -43,6 +43,16 @@ struct child_process {
> unsigned stdout_to_stderr:1;
> unsigned use_shell:1;
> unsigned clean_on_exit:1;
> + /*
> + * clean_on_exit_timeout is only considered if clean_on_exit is set.
> + * - Specify 0 to kill the child on Git exit (default)
> + * - Specify a negative value to close the child's stdin on Git exit
> + * and wait indefinitely for the child's termination.
> + * - Specify a positive value to close the child's stdin on Git exit
> + * and wait clean_on_exit_timeout seconds for the child's
> + * termination.
All right, so here is this documentation...
> + */
> + int clean_on_exit_timeout;
> };
>
> #define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }
>
>
For full patch, you would need also to add to Documentation/config.txt
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: Reference a submodule branch instead of a commit
From: Junio C Hamano @ 2016-10-04 19:01 UTC (permalink / raw)
To: Stefan Beller; +Cc: Heiko Voigt, Jeremy Morton, git@vger.kernel.org
In-Reply-To: <xmqqshscuilh.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Stefan Beller <sbeller@google.com> writes:
>
>> I wonder if we could make that convenient for users by not tracking
>> the submodule,
>> i.e.
>> * we have the information in the .gitmodules file
>> * the path itself is in the .gitignore
>> * no tree entry
>>
>> Then you can update to the remote latest branch, without Git reporting
>> a dirty submodule locally, in fact it reports nothing for the submodule.
>>
>> It sounds like a hack, but maybe it's worth looking into that when
>> people want to see that workflow.
>
> It IS a hack.
>
> But if you do not touch .git<anything> file and instead say "clone
> this other project at that path yourself" in README, that would
> probably be sufficient.
eh,... hit send too early.
It IS a hack, but having this information in .git<something> would
mean that it can be forced to be in machine readable form, unlike a
mention in README. I do not know if the .gitmodules/.gitignore
combination is a sensible thing to use, but it does smell like a
potentially useful hack.
^ permalink raw reply
* Re: [PATCH v3 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c
From: Junio C Hamano @ 2016-10-04 18:56 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <cover.1475586229.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> This is the 5th last patch series of my work to accelerate interactive
> rebases in particular on Windows.
Offtopic, but I am always confused by what you might mean by this
"nth last patch series". Is this series 5th from the last and we
have four more to go?
In any case, after a quick re-read and comparison with the last
round, I think this is in a good shape. I'd say that we would wait
for a few days for others to comment and then merge it to 'next' if
we missed nothing glaringly wrong.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 4/6] Export also the has_un{staged,committed}_changed() functions
From: Junio C Hamano @ 2016-10-04 18:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <017586232230ad87dd7cde5801e011cce9255bc0.1475586229.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> They will be used in the upcoming rebase helper.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
Makes sense.
^ permalink raw reply
* Re: [PATCH v3 3/6] Make the require_clean_work_tree() function reusable
From: Junio C Hamano @ 2016-10-04 18:52 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <3b4f46b761589d84b7713c869d00d3231ab346fd.1475586229.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> It is remarkable that libgit.a did not sport this function yet... Let's
> move it into a more prominent (and into an actually reusable) spot:
> wt-status.[ch].
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
Thanks.
I'd tweak the message while queuing, though.
wt-status: make the require_clean_work_tree() function reusable
The function "git pull" uses to stop the user when the working
tree has changes is useful in other places.
Let's move it into a more prominent (and into an actually reusable)
spot: wt-status.[ch].
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Readers won't care whether you found something remarkable and even
if they wanted to care, it is rather hard to sympathize with you
unless they know what "this function" does well enough to understand
why it is a good thing to make it available more widely. That is a
more important point to address in the log message.
> diff --git a/wt-status.c b/wt-status.c
> index 9628c1d..b92c54d 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -16,6 +16,7 @@
> #include "strbuf.h"
> #include "utf8.h"
> #include "worktree.h"
> +#include "lockfile.h"
Makes sense. We'd be opportunistically refreshing the index.
^ permalink raw reply
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Junio C Hamano @ 2016-10-04 18:28 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Christian Couder
In-Reply-To: <xmqqwphouivf.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> A block of lines that appear as the last paragraph in a commit
> message is a trailer block if and only if certain number or
> percentage of lines are non-garbage lines according to the above
> definition.
> ...
> I wonder if we can share a new helper function to do the detection
> (and classification) of a trailer block and parsing the logical
> lines out of a commit log message. The function signature could be
> as simple as taking a single <const char *> (or a strbuf) that holds
> a commit log message, and splitting it out into something like:
>
> struct {
> const char *whole;
> const char *end_of_message_proper;
> struct {
> const char *token;
> const char *contents;
> } *trailer;
> int alloc_trailers, nr_trailers;
> };
>
> where
> ...
An addendum. We may also want to be prepared to accept an input
that has some garbage lines _after_ the trailer block, if we can
clearly identify them as such. For example, we could change the
definition of "the last paragraph" as "the block of lines that
do not have any empty (or blank) line, that appear either at the end
of the input, or immediately before three-dash lines", to allow
commit title
explanation of the change
Signed-off-by: Some Body <some@body.xz>
[some other things]
Acked-by: Some Other Person <some@other.xz>
---
additional comment
which (unfortunately) is a rather common pattern for people who plan
to send the commit over e-mail.
If we add a new field "const char *beginning_of_tail_garbage" next
to "end_of_message_proper" that points at the blank line before the
three-dash line in the above example, the parser should be able to
break such an input into a parsed form, allow the trailer[] array to
be manipulated and reproduce a commit log message.
^ permalink raw reply
* Re: [PATCH] push: change submodule default to check
From: Jeff King @ 2016-10-04 18:28 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, git@vger.kernel.org, Heiko Voigt, Linus Torvalds
In-Reply-To: <CAGZ79kYQsXxzXihtzC70Oj29RAe=8+nuJeUWNmbF17C75xbEag@mail.gmail.com>
On Tue, Oct 04, 2016 at 11:08:33AM -0700, Stefan Beller wrote:
> On Tue, Oct 4, 2016 at 11:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Jeff King <peff@peff.net> writes:
> >
> >> Actually, I like that a bit better. It would not cover the case where
> >> you have not actually checked out any of the submodules (or at least not
> >> called "submodule init", I guess?). But arguably that is a sign that the
> >> auto-recurse behavior should not be kicking in anyway.
> >
> > Yeah, the "no init, no recursion" line of thought is very sensible.
> > I like it.
>
> Bear in mind that "submodule init" only fuzzes around with .git/config.
> It doesn't touch .git/modules (i.e. cloning/fetching), that is to be done
> with the update command.
>
> So if we also want to cover the case of init'd submodules, but not
> cloned/checked out submodules, we'd rather want to consult .git/config
> whether there is any submodule.* option set, though that seems more
> expensive than just checking for files inside the modules directory.
Consulting .git/config is fine, I think. It's not like we don't read it
(sometimes multiple times!) during the normal course of the program
anyway. It's just a question of whether it makes more sense for the
heuristic to kick in after "init", or only after "update". I don't know
enough to have an opinion.
-Peff
^ permalink raw reply
* Re: [PATCH] push: change submodule default to check
From: Stefan Beller @ 2016-10-04 18:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, git@vger.kernel.org, Heiko Voigt, Linus Torvalds
In-Reply-To: <xmqq7f9ouh1t.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 4, 2016 at 11:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> Actually, I like that a bit better. It would not cover the case where
>> you have not actually checked out any of the submodules (or at least not
>> called "submodule init", I guess?). But arguably that is a sign that the
>> auto-recurse behavior should not be kicking in anyway.
>
> Yeah, the "no init, no recursion" line of thought is very sensible.
> I like it.
>
Bear in mind that "submodule init" only fuzzes around with .git/config.
It doesn't touch .git/modules (i.e. cloning/fetching), that is to be done
with the update command.
So if we also want to cover the case of init'd submodules, but not
cloned/checked out submodules, we'd rather want to consult .git/config
whether there is any submodule.* option set, though that seems more
expensive than just checking for files inside the modules directory.
^ permalink raw reply
* Re: [PATCH] push: change submodule default to check
From: Stefan Beller @ 2016-10-04 18:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, git@vger.kernel.org, Heiko Voigt, Linus Torvalds
In-Reply-To: <xmqqfuocuh92.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 4, 2016 at 11:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +static void preset_submodule_default(void)
>> +{
>> + if (file_exists(".gitmodules"))
>
> Don't we need to see if we are in a bare repository?
See discussion with Jeff; instead of checking the file, we rather want to check
if $GIT_DIR/modules/ is populated, as that is version agnostic
("Was a submodule initialized and fetched at any time in the
life time of this repository?"), as well as bare/non-bare agnostic.
>
>> + recurse_submodules = RECURSE_SUBMODULES_CHECK;
>> + else
>> + recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
>
> Hmph, why "_DEFAULT" not "_OFF"?
You answered yourself below, and that was indeed my thought. I was
also wondering
whether to remove the else, but then I thought that we'd rather do not
want to rely on
compiled-in at all, and have one init function which sets out the new
world order.
^ permalink raw reply
* Re: [PATCH] push: change submodule default to check
From: Junio C Hamano @ 2016-10-04 18:04 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git@vger.kernel.org, Heiko Voigt, Linus Torvalds
In-Reply-To: <20161004175449.gn5cw6wcbvloqkzj@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Actually, I like that a bit better. It would not cover the case where
> you have not actually checked out any of the submodules (or at least not
> called "submodule init", I guess?). But arguably that is a sign that the
> auto-recurse behavior should not be kicking in anyway.
Yeah, the "no init, no recursion" line of thought is very sensible.
I like it.
^ permalink raw reply
* Re: [PATCH] push: change submodule default to check
From: Junio C Hamano @ 2016-10-04 18:02 UTC (permalink / raw)
To: Stefan Beller; +Cc: peff, git, hvoigt, torvalds
In-Reply-To: <xmqqfuocuh92.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Stefan Beller <sbeller@google.com> writes:
>
>> +static void preset_submodule_default(void)
>> +{
>> + if (file_exists(".gitmodules"))
>
> Don't we need to see if we are in a bare repository?
>
>> + recurse_submodules = RECURSE_SUBMODULES_CHECK;
>> + else
>> + recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
>
> Hmph, why "_DEFAULT" not "_OFF"?
... because you wanted to keep the same behaviour, i.e. keep
recurse_submodules set to _DEFAULT just like the compiled-in
initialization does.
Perhaps we can lose "else" clause altogether?
^ permalink raw reply
* Re: Regression: git no longer works with musl libc's regex impl
From: Ray Donnelly @ 2016-10-04 18:00 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Rich Felker, Jeff King, git, musl
In-Reply-To: <alpine.DEB.2.20.1610041915320.35196@virtualbox>
On Tue, Oct 4, 2016 at 6:16 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Rich,
>
> On Tue, 4 Oct 2016, Rich Felker wrote:
>
>> On Tue, Oct 04, 2016 at 06:08:33PM +0200, Johannes Schindelin wrote:
>> > Hi Rich,
>> >
>> > On Tue, 4 Oct 2016, Rich Felker wrote:
>> >
>> > > On Tue, Oct 04, 2016 at 11:27:22AM -0400, Jeff King wrote:
>> > > > On Tue, Oct 04, 2016 at 11:08:48AM -0400, Rich Felker wrote:
>> > > >
>> > > > > 1. is nonzero mod page size, it just works; the remainder of the last
>> > > > > page reads as zero bytes when mmapped.
>> > > >
>> > > > Is that a portable assumption?
>> > >
>> > > Yes.
>> >
>> > No, it is not. You quote POSIX, but the matter of the fact is that we use
>> > a subset of POSIX in order to be able to keep things running on Windows.
>> >
>> > And quite honestly, there are lots of reasons to keep things running on
>> > Windows, and even to favor Windows support over musl support. Over four
>> > million reasons: the Git for Windows users.
>>
>> I would hope that in the future, git-for-windows users will be using
>> musl, via midipix, rather than the painfully slow and awful version
>> they're stuck with now...
>
> Git for Windows actually uses the MSVC runtime, which is blazing fast.
>
> You are probably confusing Git for Windows with Cygwin Git.
To be fair, Cygwin Git isn't *that* slow, though I look forward to the
day when MSYS2 can use the native-Windows/GfW version instead
(including your rebase-in-C changes)
>
> Ciao,
> Johannes
^ permalink raw reply
* Re: [PATCH] push: change submodule default to check
From: Junio C Hamano @ 2016-10-04 18:00 UTC (permalink / raw)
To: Stefan Beller; +Cc: peff, git, hvoigt, torvalds
In-Reply-To: <20161004164036.6584-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> +static void preset_submodule_default(void)
> +{
> + if (file_exists(".gitmodules"))
Don't we need to see if we are in a bare repository?
> + recurse_submodules = RECURSE_SUBMODULES_CHECK;
> + else
> + recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
Hmph, why "_DEFAULT" not "_OFF"?
> +}
> +
> static void add_refspec(const char *ref)
> {
> refspec_nr++;
> @@ -552,6 +560,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
> };
>
> packet_trace_identity("push");
> + preset_submodule_default();
> git_config(git_push_config, &flags);
> argc = parse_options(argc, argv, prefix, options, push_usage, 0);
> set_push_cert_flags(&flags, push_cert);
^ permalink raw reply
* Re: color.diff.whitespace unused on removed lines
From: Junio C Hamano @ 2016-10-04 17:57 UTC (permalink / raw)
To: Jeff King; +Cc: Sandro Santilli, git
In-Reply-To: <20161004161323.53qec37i2tujaxcy@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Oct 04, 2016 at 05:35:23PM +0200, Sandro Santilli wrote:
>
>> > We later did b8767f7 (diff.c: --ws-error-highlight=<kind> option,
>> > 2015-05-26) to let you see them on other lines, though. I think that
>> > would do what you want.
>>
>> Thanks, it does do what I want.
>> Any chance to specify it in the config file that I want it
>> always to behave in a certain way ?
>
> No, I don't think there's currently a matching config option. You can
> use an alias, or propose a patch to add a config option.
The final shape of such a patch would include something like the
attached. It would need to be split into a few patches and then get
additional tests and documentation written, so I won't be committing
it myself in this shape.
diff.c | 84 +++++++++++++++++++++++++++++++++++++++---------------------------
1 file changed, 50 insertions(+), 34 deletions(-)
diff --git a/diff.c b/diff.c
index a178ed39bc..a2193c3aea 100644
--- a/diff.c
+++ b/diff.c
@@ -43,6 +43,7 @@ static int diff_stat_graph_width;
static int diff_dirstat_permille_default = 30;
static struct diff_options default_diff_options;
static long diff_algorithm;
+static unsigned ws_error_highlight_default = WSEH_NEW;
static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_RESET,
@@ -172,6 +173,42 @@ long parse_algorithm_value(const char *value)
return -1;
}
+static int parse_one_token(const char **arg, const char *token)
+{
+ const char *rest;
+ if (skip_prefix(*arg, token, &rest) && (!*rest || *rest == ',')) {
+ *arg = rest;
+ return 1;
+ }
+ return 0;
+}
+
+static int parse_ws_error_highlight(const char *arg)
+{
+ const char *orig_arg = arg;
+ unsigned val = 0;
+ while (*arg) {
+ if (parse_one_token(&arg, "none"))
+ val = 0;
+ else if (parse_one_token(&arg, "default"))
+ val = WSEH_NEW;
+ else if (parse_one_token(&arg, "all"))
+ val = WSEH_NEW | WSEH_OLD | WSEH_CONTEXT;
+ else if (parse_one_token(&arg, "new"))
+ val |= WSEH_NEW;
+ else if (parse_one_token(&arg, "old"))
+ val |= WSEH_OLD;
+ else if (parse_one_token(&arg, "context"))
+ val |= WSEH_CONTEXT;
+ else {
+ return (orig_arg - arg);
+ }
+ if (*arg)
+ arg++;
+ }
+ return val;
+}
+
/*
* These are to give UI layer defaults.
* The core-level commands such as git-diff-files should
@@ -254,6 +291,11 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp(var, "diff.wserrorhighlight")) {
+ ws_error_highlight_default = parse_ws_error_highlight(value);
+ return 0;
+ }
+
if (git_diff_heuristic_config(var, value, cb) < 0)
return -1;
if (git_color_config(var, value, cb) < 0)
@@ -3307,7 +3349,7 @@ void diff_setup(struct diff_options *options)
options->rename_limit = -1;
options->dirstat_permille = diff_dirstat_permille_default;
options->context = diff_context_default;
- options->ws_error_highlight = WSEH_NEW;
+ options->ws_error_highlight = ws_error_highlight_default;
DIFF_OPT_SET(options, RENAME_EMPTY);
/* pathchange left =NULL by default */
@@ -3698,40 +3740,14 @@ static void enable_patch_output(int *fmt) {
*fmt |= DIFF_FORMAT_PATCH;
}
-static int parse_one_token(const char **arg, const char *token)
+static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *arg)
{
- const char *rest;
- if (skip_prefix(*arg, token, &rest) && (!*rest || *rest == ',')) {
- *arg = rest;
- return 1;
- }
- return 0;
-}
+ int val = parse_ws_error_highlight(arg);
-static int parse_ws_error_highlight(struct diff_options *opt, const char *arg)
-{
- const char *orig_arg = arg;
- unsigned val = 0;
- while (*arg) {
- if (parse_one_token(&arg, "none"))
- val = 0;
- else if (parse_one_token(&arg, "default"))
- val = WSEH_NEW;
- else if (parse_one_token(&arg, "all"))
- val = WSEH_NEW | WSEH_OLD | WSEH_CONTEXT;
- else if (parse_one_token(&arg, "new"))
- val |= WSEH_NEW;
- else if (parse_one_token(&arg, "old"))
- val |= WSEH_OLD;
- else if (parse_one_token(&arg, "context"))
- val |= WSEH_CONTEXT;
- else {
- error("unknown value after ws-error-highlight=%.*s",
- (int)(arg - orig_arg), orig_arg);
- return 0;
- }
- if (*arg)
- arg++;
+ if (val < 0) {
+ error("unknown value after ws-error-highlight=%.*s",
+ -val, arg);
+ return 0;
}
opt->ws_error_highlight = val;
return 1;
@@ -3950,7 +3966,7 @@ int diff_opt_parse(struct diff_options *options,
else if (skip_prefix(arg, "--submodule=", &arg))
return parse_submodule_opt(options, arg);
else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
- return parse_ws_error_highlight(options, arg);
+ return parse_ws_error_highlight_opt(options, arg);
/* misc options */
else if (!strcmp(arg, "-z"))
^ permalink raw reply related
* Re: [PATCH v6 4/4] ls-files: add pathspec matching for submodules
From: Stefan Beller @ 2016-10-04 17:56 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jeff King, Junio C Hamano
In-Reply-To: <1475185723-36871-5-git-send-email-bmwill@google.com>
On Thu, Sep 29, 2016 at 2:48 PM, Brandon Williams <bmwill@google.com> wrote:
> Pathspecs can be a bit tricky when trying to apply them to submodules.
> The main challenge is that the pathspecs will be with respect to the
> superproject and not with respect to paths in the submodule. The
> approach this patch takes is to pass in the identical pathspec from the
> superproject to the submodule in addition to the submodule-prefix, which
> is the path from the root of the superproject to the submodule, and then
> we can compare an entry in the submodule prepended with the
> submodule-prefix to the pathspec in order to determine if there is a
> match.
>
> This patch also permits the pathspec logic to perform a prefix match against
> submodules since a pathspec could refer to a file inside of a submodule.
> Due to limitations in the wildmatch logic, a prefix match is only done
> literally. If any wildcard character is encountered we'll simply punt
> and produce a false positive match. More accurate matching will be done
> once inside the submodule. This is due to the superproject not knowing
> what files could exist in the submodule.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> Documentation/git-ls-files.txt | 3 +-
> builtin/ls-files.c | 27 +++++++--
> dir.c | 46 +++++++++++++-
> dir.h | 4 ++
> t/t3007-ls-files-recurse-submodules.sh | 108 ++++++++++++++++++++++++++++++++-
> 5 files changed, 175 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index ea01d45..51ec9a1 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -140,8 +140,7 @@ a space) at the start of each line:
>
> --recurse-submodules::
> Recursively calls ls-files on each submodule in the repository.
> - Currently there is only support for the --cached mode without a
> - pathspec.
> + Currently there is only support for the --cached.
s/--cached/--cached mode/ ?
The "the" in front of --cached sounds a bit strange for a non native
speaker here.
> + /*
> + * Find common prefix for all pathspec's
> + * This is used as a performance optimization which unfortunately cannot
> + * be done when recursing into submodules
> + */
> + if (recurse_submodules)
> + max_prefix = NULL;
> + else
> + max_prefix = common_prefix(&pathspec);
Nit of the day:
While this is readable, you may want to explore how this reads shorter as
max_prefix = recurse_submodules ? NULL : common_prefix(&pathspec);
?
> + git ls-files --recurse-submodules "sub" >actual &&
> + test_cmp expect actual &&
> + git ls-files --recurse-submodules "sub/" >actual &&
> + test_cmp expect actual &&
> + git ls-files --recurse-submodules "sub/file" >actual &&
> + test_cmp expect actual &&
> + git ls-files --recurse-submodules "su*/file" >actual &&
> + test_cmp expect actual &&
> + git ls-files --recurse-submodules "su?/file" >actual &&
> + test_cmp expect actual
> +'
> +
> + git ls-files --recurse-submodules "s??/file" >actual &&
> + test_cmp expect actual &&
> + git ls-files --recurse-submodules "s???file" >actual &&
> + test_cmp expect actual &&
> + git ls-files --recurse-submodules "s*file" >actual &&
> + test_cmp expect actual
> '
Thanks for the tests!
^ permalink raw reply
* Re: [PATCH] push: change submodule default to check
From: Jeff King @ 2016-10-04 17:54 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, git@vger.kernel.org, Heiko Voigt, Linus Torvalds
In-Reply-To: <CAGZ79kaKOjqLBsNVSmudzLUCkOJf_CsFGE8OZZHsTmuXCfiVeg@mail.gmail.com>
On Tue, Oct 04, 2016 at 10:48:51AM -0700, Stefan Beller wrote:
> > This does seem like a reasonable heuristic. I wonder if you want to
> > confirm that we actually have a worktree (and are in it) before looking
> > at file_exists(). It's unlikely that looking at ".gitmodules" in a bare
> > repo would trigger in practice, but it does not hurt to be careful.
>
> In a bare repo we'd rather want to check for an entry of .gitmodules in HEAD ?
Yeah, I think that is the closest equivalent.
> I considered it a non issue, as I don't think many people push from
> bare repositories.
I'd also agree, and I have no problem if there simply _isn't_ an auto
heuristic for bare repos. Mostly I just thought blindly calling
file_exists() was ugly (especially after all the recent "whoops, we look
at .git/config in the wrong directory" fixes I've been doing lately).
> Here is another thought:
> .gitmodules may not exist (either in working dir or in HEADs git
> tree), so maybe the
> "correct" heuristic is to check for directories in $GIT_DIR/modules/
> That is "more correct", because it is inconceivable to change the submodule
> pointers without having the submodules checked out. (Who would do that? Why?)
Actually, I like that a bit better. It would not cover the case where
you have not actually checked out any of the submodules (or at least not
called "submodule init", I guess?). But arguably that is a sign that the
auto-recurse behavior should not be kicking in anyway.
Bearing in mind that I am not too familiar with what's normal in the
submodule world, and so might be spouting nonsense. :)
-Peff
^ permalink raw reply
* Re: [PATCH] push: change submodule default to check
From: Stefan Beller @ 2016-10-04 17:48 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, git@vger.kernel.org, Heiko Voigt, Linus Torvalds
In-Reply-To: <20161004173430.eax4ptohyonc5bw2@sigill.intra.peff.net>
On Tue, Oct 4, 2016 at 10:34 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 04, 2016 at 09:40:36AM -0700, Stefan Beller wrote:
>
>> >> Why should we even have a default different from today's? If most
>> >> repositories don't have submodules enabled at all, we can just let
>> >> those working with submodules enabled to toggle their configuration
>> >> and that is an very easy to understand solution, no?
>> >
>> > You will not see any complaint from me on that. I was taking for granted
>> > that the current default is inconvenient to submodule users, but I don't
>> > have any experience myself.
>> >
>>
>> And there I was trying to help submodule users not shoot in their foot.
>
> Sorry if my reply came off as snarky.
Yeah, sorry about starting being snarky here.
>>
>> +static void preset_submodule_default(void)
>> +{
>> + if (file_exists(".gitmodules"))
>> + recurse_submodules = RECURSE_SUBMODULES_CHECK;
>> + else
>> + recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
>> +}
>
> This does seem like a reasonable heuristic. I wonder if you want to
> confirm that we actually have a worktree (and are in it) before looking
> at file_exists(). It's unlikely that looking at ".gitmodules" in a bare
> repo would trigger in practice, but it does not hurt to be careful.
In a bare repo we'd rather want to check for an entry of .gitmodules in HEAD ?
I considered it a non issue, as I don't think many people push from
bare repositories.
So if we were to check in the HEAD tree instead of the file system, why would we
apply different rules for bare and non bare repositories? We probably
would not want
to do that, so is it reasonable to check for the .gitmodules in the HEAD tree in
general in the non bare case? I dunno, it sounds like an equally cheap heuristic
covering most cases.
Here is another thought:
.gitmodules may not exist (either in working dir or in HEADs git
tree), so maybe the
"correct" heuristic is to check for directories in $GIT_DIR/modules/
That is "more correct", because it is inconceivable to change the submodule
pointers without having the submodules checked out. (Who would do that? Why?)
>
> -Peff
^ permalink raw reply
* Re: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: Rich Felker @ 2016-10-04 17:39 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git, musl
In-Reply-To: <alpine.DEB.2.20.1610041802310.35196@virtualbox>
On Tue, Oct 04, 2016 at 06:08:33PM +0200, Johannes Schindelin wrote:
> Hi Rich,
>
> On Tue, 4 Oct 2016, Rich Felker wrote:
>
> > On Tue, Oct 04, 2016 at 11:27:22AM -0400, Jeff King wrote:
> > > On Tue, Oct 04, 2016 at 11:08:48AM -0400, Rich Felker wrote:
> > >
> > > > 1. is nonzero mod page size, it just works; the remainder of the last
> > > > page reads as zero bytes when mmapped.
> > >
> > > Is that a portable assumption?
> >
> > Yes.
>
> No, it is not. You quote POSIX, but the matter of the fact is that we use
> a subset of POSIX in order to be able to keep things running on Windows.
>
> And quite honestly, there are lots of reasons to keep things running on
> Windows, and even to favor Windows support over musl support. Over four
> million reasons: the Git for Windows users.
>
> So rather than getting into an ideological discussion about "broken"
> systems, it would be good to keep things practical, realizing that those
> users make up a very real chunk of all of Git's users.
>
> As to making NO_REGEX conditional on REG_STARTEND: you are talking about
> apples and oranges here. NO_REGEX is a Makefile flag, while REG_STARTEND
> is a C preprocessor macro.
It seems like you could just always compile the source file, and just
have it all inside #if defined(NO_REGEX) || !defined(REG_STARTEND) or
similar.
> And lastly, the best alternative would be to teach musl about
> REG_STARTEND, as it is rather useful a feature.
Maybe, but it seems fundamentally costly to support -- it's extra
state in the inner loops that imposes costly spill/reload on archs
with too few registers (x86). I'll look at doing this when we
overhaul/replace the regex implementation, and I'm happy to do some
performance-regression tests for adding it now if someone has a simple
patch (as was mentioned on the musl list).
Rich
^ permalink raw reply
* Re: [PATCH v6 1/4] git: make super-prefix option
From: Jeff King @ 2016-10-04 17:39 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, git@vger.kernel.org, Junio C Hamano
In-Reply-To: <CAGZ79kax9g-FLMhPnDBP+7wJwYT884B5bGodpopo9GKgnE6+PQ@mail.gmail.com>
On Tue, Oct 04, 2016 at 10:31:51AM -0700, Stefan Beller wrote:
> On Thu, Sep 29, 2016 at 2:48 PM, Brandon Williams <bmwill@google.com> wrote:
>
> >
> > +const char *get_super_prefix(void)
> > +{
> > + if (!super_prefix)
> > + super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
> > + return super_prefix;
> > +}
> > +
>
> As said earlier, is the following a valid thought:
>
> > The getenv() function returns a pointer to the value in the
> > environment, or NULL if there is no match.
> > So in case this is not set (when e.g. the user did not specify the
> > super prefix), we would probe it a couple of times.
> > The caching effect only occurs when the string is set. So this looks
> > like we save repetitive calls, but we do not always do that.
I think your concern is valid. If it is not set, we will do an O(n)
search through the whole environment on each call.
I also think the result of getenv() needs to be copied. In some
implementations it persists for the life of the program, but that's not
guaranteed; it may be overwritten by unrelated calls to getenv() or
setenv().
-Peff
^ permalink raw reply
* Re: [PATCH v6 1/4] git: make super-prefix option
From: Junio C Hamano @ 2016-10-04 17:35 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, git@vger.kernel.org, Jeff King
In-Reply-To: <CAGZ79kax9g-FLMhPnDBP+7wJwYT884B5bGodpopo9GKgnE6+PQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Thu, Sep 29, 2016 at 2:48 PM, Brandon Williams <bmwill@google.com> wrote:
>
>>
>> +const char *get_super_prefix(void)
>> +{
>> + if (!super_prefix)
>> + super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
>> + return super_prefix;
>> +}
>> +
>
> As said earlier, is the following a valid thought:
>
>> The getenv() function returns a pointer to the value in the
>> environment, or NULL if there is no match.
>> So in case this is not set (when e.g. the user did not specify the
>> super prefix), we would probe it a couple of times.
>> The caching effect only occurs when the string is set. So this looks
>> like we save repetitive calls, but we do not always do that.
That reading is correct. If the code wants to do the caching, it
should do so correctly. Unless we expect that some callers might
want to be able to invalidate the cache,
get_super_prefix(void)
{
static int initialized;
if (!initialized) {
super_prefix = getenv(...);
initialized = 1;
}
return super_prefix;
}
would suffice.
Thanks for careful reading.
^ permalink raw reply
* Re: [PATCH] push: change submodule default to check
From: Jeff King @ 2016-10-04 17:34 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, hvoigt, torvalds
In-Reply-To: <20161004164036.6584-1-sbeller@google.com>
On Tue, Oct 04, 2016 at 09:40:36AM -0700, Stefan Beller wrote:
> >> Why should we even have a default different from today's? If most
> >> repositories don't have submodules enabled at all, we can just let
> >> those working with submodules enabled to toggle their configuration
> >> and that is an very easy to understand solution, no?
> >
> > You will not see any complaint from me on that. I was taking for granted
> > that the current default is inconvenient to submodule users, but I don't
> > have any experience myself.
> >
>
> And there I was trying to help submodule users not shoot in their foot.
Sorry if my reply came off as snarky. I really did mean it literally. I
do not know if the end goal is good or not, so all of my discussion was
just assuming it was.
So in that vein...
> diff --git a/builtin/push.c b/builtin/push.c
> index 3bb9d6b..d7d664a 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -22,7 +22,7 @@ static int deleterefs;
> static const char *receivepack;
> static int verbosity;
> static int progress = -1;
> -static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
> +static int recurse_submodules;
> static enum transport_family family;
>
> static struct push_cas_option cas;
> @@ -31,6 +31,14 @@ static const char **refspec;
> static int refspec_nr;
> static int refspec_alloc;
>
> +static void preset_submodule_default(void)
> +{
> + if (file_exists(".gitmodules"))
> + recurse_submodules = RECURSE_SUBMODULES_CHECK;
> + else
> + recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
> +}
This does seem like a reasonable heuristic. I wonder if you want to
confirm that we actually have a worktree (and are in it) before looking
at file_exists(). It's unlikely that looking at ".gitmodules" in a bare
repo would trigger in practice, but it does not hurt to be careful.
-Peff
^ permalink raw reply
* Re: [PATCH v6 1/4] git: make super-prefix option
From: Stefan Beller @ 2016-10-04 17:31 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jeff King, Junio C Hamano
In-Reply-To: <1475185723-36871-2-git-send-email-bmwill@google.com>
On Thu, Sep 29, 2016 at 2:48 PM, Brandon Williams <bmwill@google.com> wrote:
>
> +const char *get_super_prefix(void)
> +{
> + if (!super_prefix)
> + super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
> + return super_prefix;
> +}
> +
As said earlier, is the following a valid thought:
> The getenv() function returns a pointer to the value in the
> environment, or NULL if there is no match.
> So in case this is not set (when e.g. the user did not specify the
> super prefix), we would probe it a couple of times.
> The caching effect only occurs when the string is set. So this looks
> like we save repetitive calls, but we do not always do that.
>
> + if (get_super_prefix()) {
> + die("%s doesn't support --super-prefix", argv[0]);
> + }
> +
Nit: no braces, please.
^ permalink raw reply
* Re: Reference a submodule branch instead of a commit
From: Junio C Hamano @ 2016-10-04 17:31 UTC (permalink / raw)
To: Stefan Beller; +Cc: Heiko Voigt, Jeremy Morton, git@vger.kernel.org
In-Reply-To: <CAGZ79kZWtAU6YG4Qz9_Gwk2db5L2kPCCKrN+64hMYDovRjiLRw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>>
>> We already have options to support these kinds of workflows. Look at the
>> option '--remote' for 'git submodule update'.
>>
>> You then only have to commit the submodule if you do not want to see it
>> as dirty locally, but you will always get the tip of a remote tracking
>> branch when updating.
>
> I wonder if we could make that convenient for users by not tracking
> the submodule,
> i.e.
> * we have the information in the .gitmodules file
> * the path itself is in the .gitignore
> * no tree entry
>
> Then you can update to the remote latest branch, without Git reporting
> a dirty submodule locally, in fact it reports nothing for the submodule.
>
> It sounds like a hack, but maybe it's worth looking into that when
> people want to see that workflow.
It IS a hack.
But if you do not touch .git<anything> file and instead say "clone
this other project at that path yourself" in README, that would
probably be sufficient.
^ 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