* [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
* Re: [PATCH v9 04/14] run-command: add wait_on_exit
From: Junio C Hamano @ 2016-10-04 19:30 UTC (permalink / raw)
To: larsxschneider; +Cc: git, ramsay, jnareb, j6t, tboegi, peff, mlbright
In-Reply-To: <20161004125947.67104-5-larsxschneider@gmail.com>
larsxschneider@gmail.com writes:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> The flag 'clean_on_exit' kills child processes spawned by Git on exit.
> A hard kill like this might not be desired in all cases.
>
> Add 'wait_on_exit' which closes the child's stdin on Git exit and waits
> until the child process has terminated.
>
> The flag is used in a subsequent patch.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> run-command.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++--------
> run-command.h | 3 +++
> 2 files changed, 50 insertions(+), 8 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 3269362..96c54fe 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;
> + int stdin;
> + int wait;
> struct child_to_clean *next;
> };
> static struct child_to_clean *children_to_clean;
> @@ -28,12 +31,33 @@ static int installed_child_cleanup_handler;
>
> static void cleanup_children(int sig, int in_signal)
> {
> + int status;
> + struct child_to_clean *p = children_to_clean;
> +
> + /* Close the the child's stdin as indicator that Git will exit soon */
> + while (p) {
> + if (p->wait)
> + if (p->stdin > 0)
> + close(p->stdin);
> + p = p->next;
> + }
This part and the "stdin" field feels a bit too specific to the
caller you are adding. Allowing the user of the API to specify what
clean-up cation needs to be taken in the form of a callback function
may not be that much more effort and would be more flexible and
useful, I would imagine?
^ permalink raw reply
* Re: [PATCH v9 09/14] pkt-line: add packet_write_gently()
From: Junio C Hamano @ 2016-10-04 19:33 UTC (permalink / raw)
To: larsxschneider; +Cc: git, ramsay, jnareb, j6t, tboegi, peff, mlbright
In-Reply-To: <20161004125947.67104-10-larsxschneider@gmail.com>
larsxschneider@gmail.com writes:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> packet_write_fmt_gently() uses format_packet() which lets the caller
> only send string data via "%s". That means it cannot be used for
> arbitrary data that may contain NULs.
>
> Add packet_write_gently() which writes arbitrary data and does not die
> in case of an error. The function is used by other pkt-line functions in
> a subsequent patch.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> pkt-line.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 286eb09..3fd4dc0 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -171,6 +171,22 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
> return status;
> }
>
> +static int packet_write_gently(const int fd_out, const char *buf, size_t size)
> +{
> + static char packet_write_buffer[LARGE_PACKET_MAX];
> + const size_t packet_size = size + 4;
> +
> + if (packet_size > sizeof(packet_write_buffer))
> + return error("packet write failed - data exceeds max packet size");
Hmph, in the previous round, this used to be "is the size larger
than sizeof(..) - 4?", which avoided integer overflow issue rather
nicely and more idiomatic. If size is near the size_t's max,
packet_size may wrap around to become very small, and we won't hit
this error, will we?
> + packet_trace(buf, size, 1);
> + set_packet_header(packet_write_buffer, packet_size);
> + memcpy(packet_write_buffer + 4, buf, size);
> + if (write_in_full(fd_out, packet_write_buffer, packet_size) == packet_size)
> + return 0;
> + return error("packet write failed");
> +}
> +
> void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
> {
> va_list args;
^ permalink raw reply
* Re: [PATCHv2 1/2] push: change submodule default to check when submodules exist
From: Jeff King @ 2016-10-04 19:39 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, hvoigt, torvalds
In-Reply-To: <20161004192910.30649-1-sbeller@google.com>
On Tue, Oct 04, 2016 at 12:29:09PM -0700, Stefan Beller wrote:
> 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).
Hmm, I would have thought it is the opposite; can't submodules exist in
the working tree in their own ".git" directory? I know that's the "old"
way of doing it, but I didn't know if it was totally deprecated.
Anyway, the config version is probably just:
int config_check_submodule(const char *var, const char *value, void *data)
{
if (starts_with(var, "submodule.") && ends_with(var, ".path"))
*(int *)data = 1;
return 0;
}
...
int have_submodule = 0;
git_config(config_check_submodule, &have_submodule);
But I don't care too much either way. that's just for reference.
> @@ -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))
Maybe just:
if (file_exists(git_path("modules"))
?
-Peff
^ permalink raw reply
* Re: [PATCHv2 1/2] push: change submodule default to check when submodules exist
From: Stefan Beller @ 2016-10-04 19:51 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, git@vger.kernel.org, Heiko Voigt, Linus Torvalds
In-Reply-To: <20161004193926.32w7yivkakqoadm2@sigill.intra.peff.net>
On Tue, Oct 4, 2016 at 12:39 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 04, 2016 at 12:29:09PM -0700, Stefan Beller wrote:
>
>> 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).
>
> Hmm, I would have thought it is the opposite; can't submodules exist in
> the working tree in their own ".git" directory? I know that's the "old"
> way of doing it, but I didn't know if it was totally deprecated.
Oo, right. :(
The proposed patch would not change the current behavior for a layout of
.git directories inside the submodule working trees, actually.
It would however also not have the desired effect of enabling the check for
push.
However these not-yet-deprecated layouts are likely in use by people
who know what they are doing, so maybe we can punt on that.
>
> Anyway, the config version is probably just:
>
> int config_check_submodule(const char *var, const char *value, void *data)
> {
> if (starts_with(var, "submodule.") && ends_with(var, ".path"))
s/.path/.url/ but I get the point. I do dislike this solution for
another reasons though:
In the future when worktree supports submodules we either
have the url per worktree,so we'd need to process all working tree
configs as well
or we do it the proper way, which is replacing the submodule.$name.url variable
with 2 options, one is purely used to configure the URL and the other is purely
used to indicate the existence of a submodule. Piling on the mixed use case of
urls today feels sad.
> *(int *)data = 1;
> return 0;
> }
>
> ...
> int have_submodule = 0;
> git_config(config_check_submodule, &have_submodule);
>
> But I don't care too much either way. that's just for reference.
>
>> @@ -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))
>
> Maybe just:
>
> if (file_exists(git_path("modules"))
Sounds good.
So I'll see if I can get the version running you propose here, otherwise
I'll resend with these changes.
>
> ?
>
> -Peff
^ permalink raw reply
* Re: [PATCH v9 10/14] pkt-line: add functions to read/write flush terminated packet streams
From: Junio C Hamano @ 2016-10-04 19:53 UTC (permalink / raw)
To: larsxschneider; +Cc: git, ramsay, jnareb, j6t, tboegi, peff, mlbright
In-Reply-To: <20161004125947.67104-11-larsxschneider@gmail.com>
larsxschneider@gmail.com writes:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> write_packetized_from_fd() and write_packetized_from_buf() write a
> stream of packets. All content packets use the maximal packet size
> except for the last one. After the last content packet a `flush` control
> packet is written.
>
> read_packetized_to_strbuf() reads arbitrary sized packets until it
> detects a `flush` packet.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> pkt-line.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> pkt-line.h | 8 ++++++++
> 2 files changed, 77 insertions(+)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 3fd4dc0..8ffde22 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -196,6 +196,47 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
> va_end(args);
> }
>
> +int write_packetized_from_fd(int fd_in, int fd_out)
> +{
> + static char buf[LARGE_PACKET_DATA_MAX];
> + int err = 0;
> + ssize_t bytes_to_write;
> +
> + while (!err) {
> + bytes_to_write = xread(fd_in, buf, sizeof(buf));
> + if (bytes_to_write < 0)
> + return COPY_READ_ERROR;
> + if (bytes_to_write == 0)
> + break;
> + err = packet_write_gently(fd_out, buf, bytes_to_write);
> + }
> + if (!err)
> + err = packet_flush_gently(fd_out);
> + return err;
> +}
OK.
> +int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
> +{
> + static char buf[LARGE_PACKET_DATA_MAX];
> + int err = 0;
> + size_t bytes_written = 0;
> + size_t bytes_to_write;
> +
> + while (!err) {
> + if ((len - bytes_written) > sizeof(buf))
> + bytes_to_write = sizeof(buf);
> + else
> + bytes_to_write = len - bytes_written;
> + if (bytes_to_write == 0)
> + break;
> + err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
> + bytes_written += bytes_to_write;
> + }
> + if (!err)
> + err = packet_flush_gently(fd_out);
> + return err;
> +}
Hmph, what is buf[] used for, other than its sizeof() taken to yield
a constant LARGE_PACKET_DATA_MAX?
> @@ -305,3 +346,31 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
> {
> return packet_read_line_generic(-1, src, src_len, dst_len);
> }
> +
> +ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
> +{
> + int packet_len;
> +
> + size_t orig_len = sb_out->len;
> + size_t orig_alloc = sb_out->alloc;
> +
> + for (;;) {
> + strbuf_grow(sb_out, LARGE_PACKET_DATA_MAX);
> + packet_len = packet_read(fd_in, NULL, NULL,
> + // TODO: explain + 1
No // C99 comment please.
And I agree that the +1 needs to be explained.
> + sb_out->buf + sb_out->len, LARGE_PACKET_DATA_MAX+1,
> + PACKET_READ_GENTLE_ON_EOF);
> + if (packet_len <= 0)
> + break;
Hmph. So at the end of a data stream, we ask packet_read() to read
64kB or so, packet_read() gets the packet length by calling
get_packet_data() and then another get_packet_data() reads that much
and return. What happens during the next round? The first call to
get_packet_data() in packet_read() will find that the stream has
ended and returns -1, which is stored in packet_len here? But then
the data is discarded after the loop when packet_len is negative.
I must be missing something. Is the other side always supposed to
give a flush packet or something? Perhaps that is what is happening
here. If so, I am OK with that, even though it somehow sounds a bit
wasteful.
^ permalink raw reply
* Re: [PATCH v9 10/14] pkt-line: add functions to read/write flush terminated packet streams
From: Junio C Hamano @ 2016-10-04 19:58 UTC (permalink / raw)
To: larsxschneider; +Cc: git, ramsay, jnareb, j6t, tboegi, peff, mlbright
In-Reply-To: <xmqq8tu3ubzl.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> I must be missing something. Is the other side always supposed to
> give a flush packet or something? Perhaps that is what is happening
> here. If so, I am OK with that, even though it somehow sounds a bit
> wasteful.
Ah, scratch that. What I was missing was that this channel is
designed to carry multiple interactions and delimiting with EOF
which by definition can happen only once cannot be a norm.
Expecting a flush to declare the current stream terminated is
perfectly fine.
^ permalink raw reply
* Re: [PATCH 1/3] add QSORT
From: René Scharfe @ 2016-10-04 20:31 UTC (permalink / raw)
To: Kevin Bracey, GIT Mailing-list
In-Reply-To: <57F33E12.4020900@bracey.fi>
Am 04.10.2016 um 07:28 schrieb Kevin Bracey:
> On 04/10/2016 01:00, René Scharfe wrote:
>> Am 03.10.2016 um 19:09 schrieb Kevin Bracey:
>>> As such, NULL checks can still be elided even with your change. If you
>>> effectively change your example to:
>>>
>>> if (nmemb > 1)
>>> qsort(array, nmemb, size, cmp);
>>> if (!array)
>>> printf("array is NULL\n");
>>>
>>> array may only be checked for NULL if nmemb <= 1. You can see GCC doing
>>> that in the compiler explorer - it effectively turns that into "else
>>> if".
>>
>> We don't support array == NULL together with nmemb > 1, so a segfault
>> is to be expected in such cases, and thus NULL checks can be removed
>> safely.
>>
> Possibly true in practice.
>
> But technically wrong by the C standard - behaviour is undefined if the
> qsort pointer is invalid. You can't formally expect the defined
> behaviour of a segfault when sending NULL into qsort. (Hell, maybe the
> qsort has its own NULL check and silently returns!
A qsort(3) implementation that doesn't segfault is inconvenient, but
still safe. I'm more concerned about NULL checks being removed from our
code.
> So if it's not a program error for array to be NULL and nmemb to be zero
> in your code, and you want a diagnostic for array=NULL, nmemb non-zero,
> I think you should put that diagnostic into sane_qsort as an assert or
> something, not rely on qsort's undefined behaviour being a segfault.
>
> sane_qsort(blah)
> {
> if (nmemb >= 1) {
> assert(array);
> qsort(array, nmemb, ...);
> }
> }
>
> Can't invoke undefined behaviour from NULL without triggering the
> assert. (Could still have other invalid pointers, of course).
We could do that, but I think it's not necessary. We'd get a segfault
when accessing the sorted array anyway. (If we don't look at the data
after sorting then we can get rid of the sorting step altogether.)
> Usually I am on the side of "no NULL checks", as I make the assumption
> that we will get a segfault as soon as NULL pointers are used, and those
> are generally easy to diagnose. But seeing a compiler invoking this sort
> of new trickery due to invoking undefined behaviour is making me more
> nervous about doing so...
I was shocked a bit myself when I learned about this, but let's not
panic. :)
>>> To make that check really work, you have to do:
>>>
>>> if (array)
>>> qsort(array, nmemb, size, cmp);
>>> else
>>> printf("array is NULL\n");
>>>
>>> So maybe your "sane_qsort" should be checking array, not nmemb.
>>
>> It would be safe, but arguably too much so, because non-empty arrays
>> with NULL wouldn't segfault anymore, and thus become harder to
>> identify as the programming errors they are.
> Well, you get the print. Although I guess you're worrying about the
> second if being real code, not a debugging check.
Yes, but the optimization is valid: If nmemb > 0 then array can only be
NULL if we have a bug, and then we'd get a segfault eventually. So such
checks can be removed safely.
> I must say, this is quite a courageous new optimisation from GCC. It
> strikes me as finding a language lawyer loophole that seems to have been
> intended for something else (mapping library functions directly onto
> CISCy CPU intrinsics), and using it to invent a whole new optimisation
> that seems more likely to trigger bugs than optimise any significant
> amount of code in a desirable way.
Yeah, and the bugs triggered are quite obscure in this case. But having
richer type information and thus restricting the range of possible
values for variables *can* enable useful optimizations.
> Doubly weird as there's no (standard) language support for this. I don't
> know how you'd define "my_qsort" that triggered the same optimisations.
The nonnull attribute is a GCC extension, but it's also supported by clang:
http://clang.llvm.org/docs/AttributeReference.html#nonnull-gnu-nonnull
I don't know if other compilers support it as well, or if there are
efforts underway to standardize it.
> I've seen similar
> library-knowledge-without-any-way-to-reproduce-in-user-code
> optimisations like "malloc returns a new pointer that doesn't alias with
> anything existing" (and no way to reproduce the optimisation with
> my_malloc_wrapper). But those seemed to have a clear performance
> benefit, without any obvious traps. Doubtful about this one.
Still we have to deal with it..
So let's summarize; here's the effect of a raw qsort(3) call:
array == NULL nmemb bug QSORT following NULL check
------------- ----- --- ----- --------------------
0 0 no qsort is skipped
0 >0 no qsort is skipped
1 0 no qsort is skipped (bad!)
1 >0 yes qsort is skipped
Here's what the current implementation (nmemb > 1) does:
array == NULL nmemb bug QSORT following NULL check
------------- ----- --- ----- --------------------
0 0 no noop is executed
0 1 no noop is executed
0 >1 no qsort is skipped
1 0 no noop is executed
1 1 yes noop is executed
1 >1 yes qsort is skipped
With the micro-optimization removed (nmemb > 0) the matrix gets simpler:
array == NULL nmemb bug QSORT following NULL check
------------- ----- --- ----- --------------------
0 0 no noop is executed
0 >0 no qsort is skipped
1 0 no noop is executed
1 >0 yes qsort is skipped
And with your NULL check (array != NULL) we'd get:
array == NULL nmemb bug QSORT following NULL check
------------- ----- --- ----- --------------------
0 0 no qsort reuses check result
0 >0 no qsort reuses check result
1 0 no noop reuses check result
1 >0 yes noop reuses check result
Did I get it right? AFAICS all variants (except raw qsort) are safe --
no useful NULL checks are removed, and buggy code should be noticed by
segfaults in code accessing the sorted array. So the advantage of the
current code is that it won't call qsort for nmemb <= 1. And the
advantage of checking the pointer is that the result of that check can
be reused by later checks. I think the former is more useful, but only
slightly.
René
^ permalink raw reply
* Re: [PATCH 0/18] alternate object database cleanups
From: Jacob Keller @ 2016-10-04 20:40 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161004134120.aj6oywkiy4li7aeh@sigill.intra.peff.net>
On Tue, Oct 4, 2016 at 6:41 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Oct 03, 2016 at 10:47:31PM -0700, Jacob Keller wrote:
>
>> > The number of patches is a little intimidating, but I tried hard to
>> > break the refactoring down into a sequence of obviously-correct steps.
>> > You can be the judge of my success.
>>
>> I read through them once. I'm going to re-read through them again and
>> leave any comments I had.
>
> Thanks for having the fortitude to read them all. :) After looking at
> your comments, I don't _think_ there's anything that necessitates a
> re-roll, but I'll respond to a few of them individually.
>
> -Peff
Ya, I don't either. Most of my comments were just me trying to make
sure I understood what you were doing.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 04/18] t5613: whitespace/style cleanups
From: Jacob Keller @ 2016-10-04 20:41 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161004134702.evnm6xea7y6mbppo@sigill.intra.peff.net>
On Tue, Oct 4, 2016 at 6:47 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Oct 03, 2016 at 10:52:39PM -0700, Jacob Keller wrote:
>
>> On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:
>> > Our normal test style these days puts the opening quote of
>> > the body on the description line, and indents the body with
>> > a single tab. This ancient test did not follow this.
>> >
>>
>> I was surprised you didn't do this first, but it doesn't really make a
>> difference either way. This is also a pretty straight forward
>> improvement, and I can see why you'd want to split this out to review
>> separately.
>
> I was trying to leave it to the end, to move the substantive changes up
> front (and because there _isn't_ a correct style for some of the things
> it was doing). But it just got too painful to do the "don't chdir" patch
> without updating the style. I agree it might have made more sense at the
> very beginning, but I didn't think it mattered enough to go through the
> trouble of rebasing the earlier patches (which would essentially be
> rewriting them).
>
> -Peff
Right.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jacob Keller @ 2016-10-04 20:44 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161004134853.x3zq33ywyyzgbwsy@sigill.intra.peff.net>
On Tue, Oct 4, 2016 at 6:48 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Oct 03, 2016 at 10:57:48PM -0700, Jacob Keller wrote:
>
>> > diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
>> > index 7bc1c3c..b393613 100755
>> > --- a/t/t5613-info-alternate.sh
>> > +++ b/t/t5613-info-alternate.sh
>> > @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '
>> > )
>> > '
>> >
>> > +# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
>> > +# the depth at 0 and count links, not repositories, so in a chain like:
>> > +#
>> > +# A -> B -> C -> D -> E -> F -> G -> H
>> > +# 0 1 2 3 4 5 6
>> > +#
>>
>> Ok so we count links, but wouldn't we have 5 links when we hit F, and
>> not G? Or am I missing something here?
>
> This is what I was trying to get at with the "start the depth at 0". We
> disallow a depth greater than 5, but because we start at 0-counting,
> it's really six links. I guess saying "5 as too deep" is really the
> misleading part. It should be "5 as the maximum depth".
>
> -Peff
Right, but if A is 0, then:
B = 1
C = 2
D = 3
E = 4
F = 5
G = 6 (UhOh??)
H = 7
So do you mean that *B* = 0, and C = 1??? That is not clear from this commment.
So either way it still feels like "6" links is what is allowed? Or the
first link has to not count? That's really confusing.
Basically I G is the 7th letter, not the 6th, so even if we're
subtractnig 1 it's still 6 which is 1 too deep? That means we not only
discard 0 (the first repository) but we discount the 2nd one as well?
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 08/18] link_alt_odb_entry: refactor string handling
From: Jacob Keller @ 2016-10-04 20:46 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161004135353.6ywgoxutjcbaali5@sigill.intra.peff.net>
On Tue, Oct 4, 2016 at 6:53 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Oct 03, 2016 at 11:05:42PM -0700, Jacob Keller wrote:
>
>> This definitely makes reading the following function much easier,
>> though the diff is a bit funky. I think the end result is much
>> clearer.
>
> Yeah, it's really hard to see that all of the "ent" setup is kept,
> because it moves _and_ changes its content (from pfxlen to pathbuf.len).
>
> I actually tried to split this into two patches to make the diff easier
> to read, but there are two mutually dependent changes: moving to
> pathbuf.len everywhere requires not-freeing pathbuf in the early code
> path. But if you do that and don't move all of "is it usable" checks up,
> then you have to add a bunch of new error-handling code that would just
> get ripped out in the next patch.
>
> There's definitely _some_ of that in this series already (e.g., the
> counting logic in alt_sha1_path() added by patch 14 that just gets
> ripped out in patch 15 when fill_sha1_path() learns to use a strbuf). I
> tried to balance "show each individual obvious step" with "don't make
> people review a bunch of scaffolding that's not going to be in the final
> product".
>
> -Peff
Mostly the diff is funky because of how the diff selected which chunks
moved vs how your patch described what chunks moved.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jeff King @ 2016-10-04 20:49 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xok-8vhikxkp+t8pu53YJAyUjZ0NiAwejEW2j3+eP_2Xw@mail.gmail.com>
On Tue, Oct 04, 2016 at 01:44:23PM -0700, Jacob Keller wrote:
> On Tue, Oct 4, 2016 at 6:48 AM, Jeff King <peff@peff.net> wrote:
> > On Mon, Oct 03, 2016 at 10:57:48PM -0700, Jacob Keller wrote:
> >
> >> > diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> >> > index 7bc1c3c..b393613 100755
> >> > --- a/t/t5613-info-alternate.sh
> >> > +++ b/t/t5613-info-alternate.sh
> >> > @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '
> >> > )
> >> > '
> >> >
> >> > +# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
> >> > +# the depth at 0 and count links, not repositories, so in a chain like:
> >> > +#
> >> > +# A -> B -> C -> D -> E -> F -> G -> H
> >> > +# 0 1 2 3 4 5 6
> >> > +#
> >>
> >> Ok so we count links, but wouldn't we have 5 links when we hit F, and
> >> not G? Or am I missing something here?
> >
> > This is what I was trying to get at with the "start the depth at 0". We
> > disallow a depth greater than 5, but because we start at 0-counting,
> > it's really six links. I guess saying "5 as too deep" is really the
> > misleading part. It should be "5 as the maximum depth".
> >
> > -Peff
>
> Right, but if A is 0, then:
>
> B = 1
> C = 2
> D = 3
> E = 4
> F = 5
> G = 6 (UhOh??)
> H = 7
>
> So do you mean that *B* = 0, and C = 1??? That is not clear from this commment.
No, we count links, not repositories. So the "A->B" link is "0", "B->C"
is "1", and so on.
> So either way it still feels like "6" links is what is allowed? Or the
> first link has to not count? That's really confusing.
Right, 6 links _are_ allowed. Because we count links, and because we
start the link-counting at "0" and allow through "5". The link labeled
"6" (which is really the seventh link!) is the one that is forbidden.
> Basically I G is the 7th letter, not the 6th, so even if we're
> subtractnig 1 it's still 6 which is 1 too deep? That means we not only
> discard 0 (the first repository) but we discount the 2nd one as well?
It's basically two off-by-ones from what you might think is correct. I
agree it's unintuitive, but I'm just documenting what's there. We could
change it; it's not like anybody cares about the exact value except
"deep enough", but _since_ nobody cares, I preferred not to modify the
code.
-Peff
^ permalink raw reply
* Re: [PATCH v8 11/11] convert: add filter.<driver>.process option
From: Jakub Narębski @ 2016-10-04 20:50 UTC (permalink / raw)
To: Lars Schneider
Cc: git, Jeff King, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <4DE57A65-1020-4F17-81F2-9F319834BB2D@gmail.com>
[Some of answers may get invalidated by v9]
W dniu 30.09.2016 o 20:56, Lars Schneider pisze:
>> On 27 Sep 2016, at 00:41, Jakub Narębski <jnareb@gmail.com> wrote:
>>
>> Part first of the review of 11/11.
[...]
>>> +to read a welcome response message ("git-filter-server") and exactly
>>> +one protocol version number from the previously sent list. All further
>>
>> I guess that is to provide forward-compatibility, isn't it? Also,
>> "Git expects..." probably means filter process MUST send, in the
>> RFC2119 (https://tools.ietf.org/html/rfc2119) meaning.
>
> True. I feel "expects" reads better but I am happy to change it if
> you feel strong about it.
I don't have strong opinion about this. I guess following the example
of pkt-line format description may be a good idea. We are not writing
an RFC here... but having be explicit is better than be good read :-P
>>> +
>>> +After the version negotiation Git sends a list of supported capabilities
>>> +and a flush packet.
>>
>> Is it that Git SHOULD send list of ALL supported capabilities, or is
>> it that Git SHOULD NOT send capabilities it does not support, and that
>> it MAY send only those capabilities it needs (so for example if command
>> uses only `smudge`, it may not send `clean`, so that filter driver doesn't
>> need to initialize data it would not need).
>
> "After the version negotiation Git sends a list of all capabilities that
> it supports and a flush packet."
>
> Better?
Better.
I wonder if it is a matter of current implementation, namely that at
the point of code where Git is sending capabilities it doesn't know
which of them it will be using; at least in some of cases.
Because if it would be possible for Git to not send capabilities which
it supports, but is sure that it would not be using for a given operation,
then it would be good to do that. It might be that filter driver must
do some prep work for each of capabilities, so skipping some of prep
would make git faster. Though all this is for now theoretical musings...
it might be an argument for such description of protocol so it does
not prevent Git from sending only supported capabilities it needs.
>> I wonder why it is "<capability>=true", and not "capability=<capability>".
>> Is there a case where we would want to send "<capability>=false". Or
>> is it to allow configurable / value based capabilities? Isn't it going
>> a bit too far: is there even a hind of an idea for parametrize-able
>> capability? YAGNI is a thing...
>
> Peff suggested that format and I think it is OK:
> http://public-inbox.org/git/20160803224619.bwtbvmslhuicx2qi@sigill.intra.peff.net/
It wouldn't kill you to summarize the argument, would it?
From what I understand the argument is that "<capability>=true" allows
for simplist parsing into a [intermediate] hash, while the other
proposal of using"capability=<capability>" would require something more
sophisticated. And that it is better to be explicit rather than
use synonyms / shortcuts for "<capability>".
Though one can argue that "<foo>" is synonym / shortcut for "<foo>=true";
it would not complicate parsing much.
Nb. we don't use trick of 'parse metadata to hash' in neither example,
nor filter driver used in test...
[...]
>>> +
>>> +After the filter has processed a blob it is expected to wait for
>>> +the next "key=value" list containing a command. Git will close
>>> +the command pipe on exit. The filter is expected to detect EOF
>>> +and exit gracefully on its own.
Is this still true?
>>
>> Good to have it documented.
>>
>> Anyway, as it is Git command that spawns the filter driver process,
>> assuming that the filter process doesn't daemonize itself, wouldn't
>> the operating system reap it after its parent process, that is the
>> git command it invoked, dies? So detecting EOF is good, but not
>> strictly necessary for simple filter that do not need to free
>> its resources, or can leave freeing resources to the operating
>> system? But I may be wrong here.
>
> The filter process runs independent of Git.
Ah. So without some way to tell long-lived filter process that
it can shut down, because no further data will be incoming, or
killing it by Git, it would hang indefinitely?
[...]
>>> + }
>>> + elsif ( $pkt_size > 4 ) {
>>
>> Isn't a packet of $pkt_size == 4 a valid packet, a keep-alive
>> one? Or is it forbidden?
>
> "Implementations SHOULD NOT send an empty pkt-line ("0004")."
> Source: Documentation/technical/protocol-common.txt
All right. Not that we need keep-alive packet for communication
between two processes on the same host.
Though I wonder why this rule is here...
[...]
>>> +( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
>>> +( packet_txt_read() eq ( 0, "version=2" ) ) || die "bad version";
>>> +( packet_bin_read() eq ( 1, "" ) ) || die "bad version end";
>>
>> Actually, it is overly strict. It should not fail if there
>> are other "version=3", "version=4" etc. lines.
>
> True, but I think for an example this is OK. I'll add a note
> to the file header.
Anyway it would be better to have helper subroutine to get metadata
lines (packets till flush) and parse them into array or a hash...
>>> +
>>> +while (1) {
>>> + my ($command) = packet_txt_read() =~ /^command=([^=]+)$/;
>>> + my ($pathname) = packet_txt_read() =~ /^pathname=([^=]+)$/;
>>
>> Do we require this order? If it is, is that explained in the
>> documentation?
>
> Git sends that order right now but the filter should not rely
> on that order.
...and the latter would make ignoring order of lines simpler.
Though I wonder if we can ensure in the protocol documentation
that those lines would always be send in this order.
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jacob Keller @ 2016-10-04 20:52 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161004204933.ygfhoy24g6psyf6h@sigill.intra.peff.net>
On Tue, Oct 4, 2016 at 1:49 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 04, 2016 at 01:44:23PM -0700, Jacob Keller wrote:
>
>> On Tue, Oct 4, 2016 at 6:48 AM, Jeff King <peff@peff.net> wrote:
>> > On Mon, Oct 03, 2016 at 10:57:48PM -0700, Jacob Keller wrote:
>> >
>> >> > diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
>> >> > index 7bc1c3c..b393613 100755
>> >> > --- a/t/t5613-info-alternate.sh
>> >> > +++ b/t/t5613-info-alternate.sh
>> >> > @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '
>> >> > )
>> >> > '
>> >> >
>> >> > +# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
>> >> > +# the depth at 0 and count links, not repositories, so in a chain like:
>> >> > +#
>> >> > +# A -> B -> C -> D -> E -> F -> G -> H
>> >> > +# 0 1 2 3 4 5 6
>> >> > +#
>> >>
>> >> Ok so we count links, but wouldn't we have 5 links when we hit F, and
>> >> not G? Or am I missing something here?
>> >
>> > This is what I was trying to get at with the "start the depth at 0". We
>> > disallow a depth greater than 5, but because we start at 0-counting,
>> > it's really six links. I guess saying "5 as too deep" is really the
>> > misleading part. It should be "5 as the maximum depth".
>> >
>> > -Peff
>>
>> Right, but if A is 0, then:
>>
>> B = 1
>> C = 2
>> D = 3
>> E = 4
>> F = 5
>> G = 6 (UhOh??)
>> H = 7
>>
>> So do you mean that *B* = 0, and C = 1??? That is not clear from this commment.
>
> No, we count links, not repositories. So the "A->B" link is "0", "B->C"
> is "1", and so on.
>
If you need to re-roll for some other reason I would add some spaces
around the numbers so they line up better with the links so that this
becomes more clear.
>> So either way it still feels like "6" links is what is allowed? Or the
>> first link has to not count? That's really confusing.
>
> Right, 6 links _are_ allowed. Because we count links, and because we
> start the link-counting at "0" and allow through "5". The link labeled
> "6" (which is really the seventh link!) is the one that is forbidden.
Right. Ok this makes more sense now.
>
>> Basically I G is the 7th letter, not the 6th, so even if we're
>> subtractnig 1 it's still 6 which is 1 too deep? That means we not only
>> discard 0 (the first repository) but we discount the 2nd one as well?
>
> It's basically two off-by-ones from what you might think is correct. I
> agree it's unintuitive, but I'm just documenting what's there. We could
> change it; it's not like anybody cares about the exact value except
> "deep enough", but _since_ nobody cares, I preferred not to modify the
> code.
>
I agree I don't think changing code is necessary, I was just confused
by the comment that tried to make it clear.
Thanks,
Jake
> -Peff
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jeff King @ 2016-10-04 20:55 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xo3nxy1EOjDqHvKQuK128c=b73XN=6qqn6g6oRGh2VdFg@mail.gmail.com>
On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:
> >> >> > +# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
> >> >> > +# the depth at 0 and count links, not repositories, so in a chain like:
> >> >> > +#
> >> >> > +# A -> B -> C -> D -> E -> F -> G -> H
> >> >> > +# 0 1 2 3 4 5 6
> >> >> > +#
> [...]
> > No, we count links, not repositories. So the "A->B" link is "0", "B->C"
> > is "1", and so on.
>
> If you need to re-roll for some other reason I would add some spaces
> around the numbers so they line up better with the links so that this
> becomes more clear.
Hmm. Now I am puzzled, because I _did_ line up them specifically to make
this clear. I put the numbers under the ">" of the arrow. Did I screw up
the spacing somehow so that isn't how they look to you? Or are you just
saying you would prefer them under the "-" of the arrow?
-Peff
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Stefan Beller @ 2016-10-04 20:58 UTC (permalink / raw)
To: Jeff King; +Cc: Jacob Keller, Git mailing list, René Scharfe
In-Reply-To: <20161004205510.6bhisw7ixbgcvvwn@sigill.intra.peff.net>
On Tue, Oct 4, 2016 at 1:55 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:
>
>> >> >> > +# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
>> >> >> > +# the depth at 0 and count links, not repositories, so in a chain like:
>> >> >> > +#
>> >> >> > +# A -> B -> C -> D -> E -> F -> G -> H
>> >> >> > +# 0 1 2 3 4 5 6
>> >> >> > +#
>> [...]
>> > No, we count links, not repositories. So the "A->B" link is "0", "B->C"
>> > is "1", and so on.
>>
>> If you need to re-roll for some other reason I would add some spaces
>> around the numbers so they line up better with the links so that this
>> becomes more clear.
>
> Hmm. Now I am puzzled, because I _did_ line up them specifically to make
> this clear. I put the numbers under the ">" of the arrow. Did I screw up
> the spacing somehow so that isn't how they look to you? Or are you just
> saying you would prefer them under the "-" of the arrow?
>
> -Peff
Input from a self-claimed design expert for ASCII art. ;)
What about this?
# A -0-> B -1-> C -2-> ...
(Double space between letter and arrow, number included in the arrow)
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jeff King @ 2016-10-04 21:00 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jacob Keller, Git mailing list, René Scharfe
In-Reply-To: <CAGZ79kap2ndp=FK4YdqrL4tJ8_VDuuAcSCk1dtX5X2H3aaj6kQ@mail.gmail.com>
On Tue, Oct 04, 2016 at 01:58:54PM -0700, Stefan Beller wrote:
> On Tue, Oct 4, 2016 at 1:55 PM, Jeff King <peff@peff.net> wrote:
> > On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:
> >
> >> >> >> > +# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
> >> >> >> > +# the depth at 0 and count links, not repositories, so in a chain like:
> >> >> >> > +#
> >> >> >> > +# A -> B -> C -> D -> E -> F -> G -> H
> >> >> >> > +# 0 1 2 3 4 5 6
> >> >> >> > +#
> >> [...]
> >> > No, we count links, not repositories. So the "A->B" link is "0", "B->C"
> >> > is "1", and so on.
> >>
> >> If you need to re-roll for some other reason I would add some spaces
> >> around the numbers so they line up better with the links so that this
> >> becomes more clear.
> >
> > Hmm. Now I am puzzled, because I _did_ line up them specifically to make
> > this clear. I put the numbers under the ">" of the arrow. Did I screw up
> > the spacing somehow so that isn't how they look to you? Or are you just
> > saying you would prefer them under the "-" of the arrow?
> >
> > -Peff
>
> Input from a self-claimed design expert for ASCII art. ;)
> What about this?
>
> # A -0-> B -1-> C -2-> ...
>
> (Double space between letter and arrow, number included in the arrow)
I actually find that quite confusing, as it looks like "-1", "-2", etc.
This has got to be my favorite bikeshed discussion of all time, though. :)
-Peff
^ permalink raw reply
* Re: [PATCH v8 11/11] convert: add filter.<driver>.process option
From: Jakub Narębski @ 2016-10-04 21:00 UTC (permalink / raw)
To: Lars Schneider
Cc: git, Jeff King, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <EF3723DE-B34A-4314-94C9-E3EB38EAB92A@gmail.com>
[Some of answers and comments may got invalidated by v9]
W dniu 30.09.2016 o 21:38, Lars Schneider pisze:
>> On 27 Sep 2016, at 17:37, Jakub Narębski <jnareb@gmail.com> wrote:
>>
>> Part second of the review of 11/11.
[...]
>>> +
>>> + if (start_command(process)) {
>>> + error("cannot fork to run external filter '%s'", cmd);
>>> + kill_multi_file_filter(hashmap, entry);
>>> + return NULL;
>>> + }
>>
>> I guess there is a reason why we init hashmap entry, try to start
>> external process, then kill entry of unable to start, instead of
>> trying to start external process, and adding hashmap entry when
>> we succeed?
>
> Yes. This way I can reuse the kill_multi_file_filter() function.
I don't quite understand. If you didn't fill the entry before
using start_command(process), you would not need kill_multi_file_filter(),
which in that case IIUC just removes the just created entry from hashmap.
Couldn't you add entry to hashmap in the 'else' part? Or would it
be racy?
[...]
>>> +static void read_multi_file_filter_values(int fd, struct strbuf *status) {
>>
>> This is more
>>
>> +static void read_multi_file_filter_status(int fd, struct strbuf *status) {
>>
>> It doesn't read arbitrary values, it examines 'metadata' from
>> filter for "status=<foo>" lines.
>
> True!
>
>>> + if (pair[0] && pair[0]->len && pair[1]) {
>>> + if (!strcmp(pair[0]->buf, "status=")) {
>>> + strbuf_reset(status);
>>> + strbuf_addbuf(status, pair[1]);
>>> + }
>>
>> So it is last status=<foo> line wins behavior?
>
> Correct.
Perhaps this should be described in code comment.
>>>
>>> + fflush(NULL);
>>
>> Why this fflush(NULL) is needed here?
>
> This flushes all open output streams. The single filter does the same.
I know what it does, but I don't know why. But "single filter does it"
is good enough for me. Still would want to know why, though ;-)
>>>
>>> + if (fd >= 0 && !src) {
>>> + if (fstat(fd, &file_stat) == -1)
>>> + return 0;
>>> + len = xsize_t(file_stat.st_size);
>>> + }
>>
>> Errr... is it necessary? The protocol no longer provides size=<n>
>> hint, and neither uses such hint if provided.
>
> We require the size in write_packetized_from_buf() later.
Don't we use write_packetized_from_fd() in the case of fd >= 0?
[...]
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 05/18] t5613: do not chdir in main process
From: Junio C Hamano @ 2016-10-04 21:00 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
In-Reply-To: <20161003203408.qnakqgcninzty3sr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Our usual style when working with subdirectories is to chdir
> inside a subshell or to use "git -C", which means we do not
> have to constantly return to the main test directory. Let's
> convert this old test, which does not follow that style.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t5613-info-alternate.sh | 92 +++++++++++++++++------------------------------
> 1 file changed, 33 insertions(+), 59 deletions(-)
Whew. Quite a lot of cleanups on this ancient script. Thanks.
^ permalink raw reply
* [PATCHv3 1/2] push: change submodule default to check when submodules exist
From: Stefan Beller @ 2016-10-04 21:03 UTC (permalink / raw)
To: gitster; +Cc: git, peff, hvoigt, torvalds, Stefan Beller
In-Reply-To: <CAGZ79kbt+SZoogKTV_-rVfOOFzf6xrhWytrBo2H3r6NQw34WTw@mail.gmail.com>
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,
thanks for the suggestions, both git_path(..) as well as checking the config,
this seems quite readable to me:
builtin/push.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/builtin/push.c b/builtin/push.c
index 3bb9d6b..65bb1df 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,6 +23,7 @@ static int deleterefs;
static const char *receivepack;
static int verbosity;
static int progress = -1;
+static int has_submodules;
static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
static enum transport_family family;
@@ -31,6 +33,14 @@ static const char **refspec;
static int refspec_nr;
static int refspec_alloc;
+static void preset_submodule_default(void)
+{
+ if (has_submodules || file_exists(git_path("modules")))
+ recurse_submodules = RECURSE_SUBMODULES_CHECK;
+ else
+ recurse_submodules = RECURSE_SUBMODULES_OFF;
+}
+
static void add_refspec(const char *ref)
{
refspec_nr++;
@@ -495,7 +505,8 @@ static int git_push_config(const char *k, const char *v, void *cb)
const char *value;
if (!git_config_get_value("push.recursesubmodules", &value))
recurse_submodules = parse_push_recurse_submodules_arg(k, value);
- }
+ } else if (starts_with(k, "submodule.") && ends_with(k, ".url"))
+ has_submodules = 1;
return git_default_config(k, v, NULL);
}
@@ -552,6 +563,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
* Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors
From: Junio C Hamano @ 2016-10-04 21:08 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
In-Reply-To: <20161003203417.izcgwt4yz3yspdnm@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> +int strbuf_normalize_path(struct strbuf *src)
> +{
> + struct strbuf dst = STRBUF_INIT;
> +
> + strbuf_grow(&dst, src->len);
> + if (normalize_path_copy(dst.buf, src->buf) < 0) {
> + strbuf_release(&dst);
> + return -1;
> + }
> +
> + /*
> + * normalize_path does not tell us the new length, so we have to
> + * compute it by looking for the new NUL it placed
> + */
> + strbuf_setlen(&dst, strlen(dst.buf));
> + strbuf_swap(src, &dst);
> + strbuf_release(&dst);
> + return 0;
> +}
Makes sense.
^ permalink raw reply
* Re: [PATCH 08/18] link_alt_odb_entry: refactor string handling
From: Junio C Hamano @ 2016-10-04 21:18 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
In-Reply-To: <20161003203448.cdfbitl5jmhlpb5o@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The string handling in link_alt_odb_entry() is mostly an
> artifact of the original version, which took the path as a
> ptr/len combo, and did not have a NUL-terminated string
> until we created one in the alternate_object_database
> struct. But since 5bdf0a8 (sha1_file: normalize alt_odb
> path before comparing and storing, 2011-09-07), the first
> thing we do is put the path into a strbuf, which gives us
> some easy opportunities for cleanup.
>
> In particular:
>
> - we call strlen(pathbuf.buf), which is silly; we can look
> at pathbuf.len.
>
> - even though we have a strbuf, we don't maintain its
> "len" field when chomping extra slashes from the
> end, and instead keep a separate "pfxlen" variable. We
> can fix this and then drop "pfxlen" entirely.
>
> - we don't check whether the path is usable until after we
> allocate the new struct, making extra cleanup work for
> ourselves. Since we have a NUL-terminated string, we can
> bump the "is it usable" checks higher in the function.
> While we're at it, we can move that logic to its own
> helper, which makes the flow of link_alt_odb_entry()
> easier to follow.
Also I find that this bit is a nice touch:
> /* recursively add alternates */
> - read_info_alternates(ent->base, depth + 1);
> -
> - ent->base[pfxlen] = '/';
> + read_info_alternates(pathbuf.buf, depth + 1);
We used to leave ent->base[] string terminated with NUL while
calling read_info_alternates() and then added '/' after that, but
because the new code uses a separate pathbuf for the call, ent->base[]
can be prepared into the desired shape upfront.
Much easier to follow.
^ permalink raw reply
* Re: [PATCH 12/18] alternates: use a separate scratch space
From: Junio C Hamano @ 2016-10-04 21:29 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
In-Reply-To: <20161003203551.tmqp5rll6nqkewxz@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> extern struct alternate_object_database {
> struct alternate_object_database *next;
> +
> char *name;
> - char base[FLEX_ARRAY]; /* more */
> + char *scratch;
> +
> + char path[FLEX_ARRAY];
> } *alt_odb_list;
It is not wrong per-se, but I am a bit surprised to see that the
code keeps FLEX_ARRAY _and_ uses a separate malloc'ed area pointed
at by the scratch pointer.
Loss of "compare only up to the location 'name' points at" makes the
users of the struct that want only the directory path certainly a
lot simpler and easier to follow.
Thanks.
^ permalink raw reply
* Re: [PATCH 12/18] alternates: use a separate scratch space
From: Jeff King @ 2016-10-04 21:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, René Scharfe
In-Reply-To: <xmqqk2dnssz9.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 04, 2016 at 02:29:46PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > extern struct alternate_object_database {
> > struct alternate_object_database *next;
> > +
> > char *name;
> > - char base[FLEX_ARRAY]; /* more */
> > + char *scratch;
> > +
> > + char path[FLEX_ARRAY];
> > } *alt_odb_list;
>
> It is not wrong per-se, but I am a bit surprised to see that the
> code keeps FLEX_ARRAY _and_ uses a separate malloc'ed area pointed
> at by the scratch pointer.
Yeah, there's really no reason "path" could not become a non-flex
buffer. I mostly left it there out of inertia. If you have a preference,
I'm happy to change it.
-Peff
^ 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