* Re: [PATCH v2 1/3] serialize collection of changed submodules
From: Junio C Hamano @ 2016-10-10 22:43 UTC (permalink / raw)
To: Stefan Beller
Cc: Heiko Voigt, Jeff King, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <CAGZ79kZiY56-84aThH1F02E_HzCTAK3KSYLbyP1D5GUAt892cw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> +static struct sha1_array *get_sha1s_from_list(struct string_list *submodules,
>> + const char *path)
>
> So this will take the stringlist `submodules` and insert the path into it,
> if it wasn't already in there. In case it is newly inserted, add a sha1_array
> as util, so each inserted path has it's own empty array.
>
> So it is both init of the data structures as well as retrieving them. I was
> initially confused by the name as I assumed it would give you sha1s out
> of a string list (e.g. transform strings to internal sha1 things).
> Maybe it's just
> me having a hard time to understand that, but I feel like the name could be
> improved.
>
> lookup_sha1_list_by_path,
> insert_path_and_return_sha1_list ?
I do not think either the name or the "find if exists otherwise
initialize one" behaviour is particularly confusing, but I do not
think "maintain a set of sha1_arrays keyed with a string" is a so
widely reusable general concept/construct. As can be seen easily in
the names of parameters, this function is about maintaining a set of
sha1_arrays keyed by paths to submodules, and I also assume that the
array indexed by path is not meant to be a general purpose "we can
use it to store any 40-hex thing" but to store something specific.
What is that specific thing? The names of commit objects in the
submodule repository?
I'd prefer to see that exact thing used to construct the function
name for a helper function with specific usage in mind, i.e.
get_commit_object_names_for_submodule_path() or something along that
line.
^ permalink raw reply
* Re: [PATCH v3 05/25] sequencer: eventually release memory allocated for the option values
From: Junio C Hamano @ 2016-10-10 22:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <a67af02ef363311b526bddba864c7f1ca9087b43.1476120229.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 7365559..fce9c75 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -174,6 +174,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>
> if (argc > 1)
> usage_with_options(usage_str, options);
> +
> + /* These option values will be free()d */
> + if (opts->gpg_sign)
> + opts->gpg_sign = xstrdup(opts->gpg_sign);
> + if (opts->strategy)
> + opts->strategy = xstrdup(opts->strategy);
> }
This certainly is good, but I wonder if a new variant of OPT_STRING
and OPTION_STRING that does the strdup for you, something along the
lines of ...
diff --git a/parse-options.c b/parse-options.c
index 312a85dbde..6aab6b0b05 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -138,6 +138,21 @@ static int get_value(struct parse_opt_ctx_t *p,
return get_arg(p, opt, flags, (const char **)opt->value);
return 0;
+ case OPTION_STRDUP:
+ err = 0;
+ free(opt->value);
+ if (unset)
+ *(const char **)opt->value = NULL;
+ else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
+ *(const char **)opt->value = xstrdup(opt->defval);
+ else {
+ const char *v;
+ err = get_arg(p, opt, flags, &v);
+ if (!err)
+ *(const char **)opt->value = xstrdup(v);
+ }
+ return err;
+
case OPTION_FILENAME:
err = 0;
if (unset)
... may make it even more pleasant to use? Only for two fields in
this patch that may probably be an overkill, but we may eventually
benefit from such an approach when we audit and plug leaks in
parse-options users. I dunno.
It is a sign that the caller wants to _own_ the memory to mark a
variable or field with OPTION_STRDUP, which is why I added the
free() at the beginning there.
The remainder of this patch looks sensible. The code that frees
these fields when we are done with the struct (and when we are
re-assigning) looked all good.
^ permalink raw reply related
* Re: [PATCH v3 03/25] sequencer: avoid unnecessary indirection
From: Junio C Hamano @ 2016-10-10 22:14 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <336cefca0bda214b5b43b1094af13d787d1a79e3.1476120229.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> We really do not need the *pointer to a* pointer to the options in
> the read_populate_opts() function.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
I vaguely recall seeing this in the previous round and finding it
pretty sensible. And I still do ;-)
[the remainder left as-is to help those who are reading from
sidelines]
> diff --git a/sequencer.c b/sequencer.c
> index cb16cbd..c2fbf6f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -813,7 +813,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
> return 0;
> }
>
> -static int read_populate_opts(struct replay_opts **opts)
> +static int read_populate_opts(struct replay_opts *opts)
> {
> if (!file_exists(git_path_opts_file()))
> return 0;
> @@ -823,7 +823,7 @@ static int read_populate_opts(struct replay_opts **opts)
> * about this case, though, because we wrote that file ourselves, so we
> * are pretty certain that it is syntactically correct.
> */
> - if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
> + if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) < 0)
> return error(_("Malformed options sheet: %s"),
> git_path_opts_file());
> return 0;
> @@ -1054,7 +1054,7 @@ static int sequencer_continue(struct replay_opts *opts)
>
> if (!file_exists(git_path_todo_file()))
> return continue_single_pick();
> - if (read_populate_opts(&opts) ||
> + if (read_populate_opts(opts) ||
> read_populate_todo(&todo_list, opts))
> return -1;
^ permalink raw reply
* Re: [PATCH v3 01/25] sequencer: use static initializers for replay_opts
From: Junio C Hamano @ 2016-10-10 22:14 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <5c4c86f0ea7ee4a9bad15e48f72b8fe5baa72dfb.1476120229.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> This change is not completely faithful: instead of initializing all fields
> to 0, we choose to initialize command and subcommand to -1 (instead of
> defaulting to REPLAY_REVERT and REPLAY_NONE, respectively). Practically,
> it makes no difference at all, but future-proofs the code to require
> explicit assignments for both fields.
The assignments to opts.action immediately following, I would say
this is quite faithful conversion that looks good.
> int cmd_revert(int argc, const char **argv, const char *prefix)
> {
> - struct replay_opts opts;
> + struct replay_opts opts = REPLAY_OPTS_INIT;
> int res;
>
> - memset(&opts, 0, sizeof(opts));
> if (isatty(0))
> opts.edit = 1;
> opts.action = REPLAY_REVERT;
> @@ -195,10 +194,9 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
>
> int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
> {
> - struct replay_opts opts;
> + struct replay_opts opts = REPLAY_OPTS_INIT;
> int res;
>
> - memset(&opts, 0, sizeof(opts));
> opts.action = REPLAY_PICK;
> git_config(git_default_config, NULL);
> parse_args(argc, argv, &opts);
> diff --git a/sequencer.h b/sequencer.h
> index 5ed5cb1..db425ad 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -47,6 +47,7 @@ struct replay_opts {
> /* Only used by REPLAY_NONE */
> struct rev_info *revs;
> };
> +#define REPLAY_OPTS_INIT { -1, -1 }
>
> int sequencer_pick_revisions(struct replay_opts *opts);
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Junio C Hamano @ 2016-10-10 22:13 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <4e73ba3e8c1700259ffcc3224d1f66e6a760142d.1476120229.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Let's just bite the bullet and rewrite the entire parser; the code now
> ...
> In particular, we choose to maintain the list of commands in an array
> instead of a linked list: this is flexible enough to allow us later on to
> even implement rebase -i's reordering of fixup!/squash! commits very
> easily (and with a very nice speed bonus, at least on Windows).
>
> While at it, do not stop at the first problem, but list *all* of the
> problems. This will help the user when the sequencer will do `rebase
> -i`'s work by allowing to address all issues in one go rather than going
> back and forth until the todo list is valid.
All sounds sensible.
> if (parent && parse_commit(parent) < 0)
> - /* TRANSLATORS: The first %s will be "revert" or
> - "cherry-pick", the second %s a SHA1 */
> + /*
> + * TRANSLATORS: The first %s will be a "todo" command like
> + * "revert" or "pick", the second %s a SHA1.
> + */
You may want to double check this with i18n folks; IIRC the tool
that extracts TRANSLATORS: comment was somewhat particular about
where that magic "TRANSLATORS:" token resides on a comment line and
that is why we have this multi-line comment formatted in an unusual
way.
Ahh, no you do not have to bug i18n folks. 47fbfded53 ("i18n: only
extract comments marked with "TRANSLATORS:"", 2014-04-17) is an
example of such an adjustment.
I just found it in CodingGuidelines, cbcfd4e3ea ("i18n: mention
"TRANSLATORS:" marker in Documentation/CodingGuidelines",
2014-04-18).
> + while ((commit = get_revision(opts->revs))) {
> + struct todo_item *item = append_new_todo(todo_list);
> + const char *commit_buffer = get_commit_buffer(commit, NULL);
> + const char *subject;
> + int subject_len;
> +
> + item->command = command;
> + item->commit = commit;
> + item->offset_in_buf = todo_list->buf.len;
> + subject_len = find_commit_subject(commit_buffer, &subject);
> + strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
> + find_unique_abbrev(commit->object.oid.hash,
> + DEFAULT_ABBREV),
> + subject_len, subject);
I am personally fine with this line; two things come to mind:
- This would work just fine as-is with Linus's change to turn
DEFAULT_ABBREV to -1.
- It appears that it is more fashionable to use
strbuf_add_unique_abbrev() these days.
Thanks.
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Jeff King @ 2016-10-10 21:57 UTC (permalink / raw)
To: Larry Finger
Cc: Mathieu Lienard--Mayor, Jorge Juan Garcia Garcia, Matthieu Moy,
Remi Lespinet, Matthieu Moy, git
In-Reply-To: <20161010214856.fobd3jgsv2cnscs3@sigill.intra.peff.net>
[+cc authors of b1c8a11, which regressed this case; I'll quote liberally
to give context]
On Mon, Oct 10, 2016 at 05:48:56PM -0400, Jeff King wrote:
> I can't reproduce the problem with this simple setup:
>
> git init
> echo content >file && git add file
> git commit -F- <<-\EOF
> the subject
>
> the body
>
> Cc: Stable <stable@vger.kernel.org> [4.8+]
> EOF
>
> If I then run:
>
> git send-email -1 --to=peff@peff.net --dry-run
>
> I get:
>
> /tmp/MH8SfHOjCv/0001-the-subject.patch
> (mbox) Adding cc: Jeff King <peff@peff.net> from line 'From: Jeff King <peff@peff.net>'
> (body) Adding cc: Stable <stable@vger.kernel.org> [4.8+] from line 'Cc: Stable <stable@vger.kernel.org> [4.8+]'
> Dry-OK. Log says:
> Sendmail: /usr/sbin/sendmail -i peff@peff.net stable@vger.kernel.org
> From: Jeff King <peff@peff.net>
> To: peff@peff.net
> Cc: "Stable [4.8+]" <stable@vger.kernel.org>
> Subject: [PATCH] the subject
> Date: Mon, 10 Oct 2016 17:44:25 -0400
> Message-Id: <20161010214425.9761-1-peff@peff.net>
> X-Mailer: git-send-email 2.10.1.527.g93d4615
>
> Result: OK
>
> So it looks like it parsed the address, and shifted the "4.8+" bit into
> the name, which seems reasonable. Does my example behave differently on
> your system? If not, can you see what's different between your
> real-world case and the example?
>
> It might also be related to which perl modules are available. We'll use
> Mail::Address if you have it, but some fallback routines if you don't.
> They may behave differently.
>
> Alternatively, if this used to work, you might try bisecting it.
Ah, it is Mail::Address. It gets this case right, but if I uninstall it,
then the cc becomes:
Cc: Stable <stable@vger.kernel.org[4.8+]>
that you saw, which is broken. Older versions of git, even without
Mail::Address, got this right. The breakage bisects to b1c8a11
(send-email: allow multiple emails using --cc, --to and --bcc,
2015-06-30) from v2.6.0, but I didn't dig deeper into the cause.
-Peff
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Jeff King @ 2016-10-10 21:48 UTC (permalink / raw)
To: Larry Finger; +Cc: git
In-Reply-To: <41164484-309b-bfff-ddbb-55153495d41a@lwfinger.net>
On Mon, Oct 10, 2016 at 04:00:56PM -0500, Larry Finger wrote:
> I have recently switched to openSUSE Leap 42.2 and found that some of the
> features of send_mail no longer work. The problem occurs when trying to add
> information to a Cc to Stable.
>
> The initial pass through the patch produces the output
> (body) Adding cc: Stable <stable@vger.kernel.org> [4.8+] from line 'Cc:
> Stable <stable@vger.kernel.org> [4.8+]'
>
> That is correct, but the actual Cc list contains
> Stable <stable@vger.kernel.org[4.8+]>,
>
> The mangled address is not legal and the mail attempt fails.
I can't reproduce the problem with this simple setup:
git init
echo content >file && git add file
git commit -F- <<-\EOF
the subject
the body
Cc: Stable <stable@vger.kernel.org> [4.8+]
EOF
If I then run:
git send-email -1 --to=peff@peff.net --dry-run
I get:
/tmp/MH8SfHOjCv/0001-the-subject.patch
(mbox) Adding cc: Jeff King <peff@peff.net> from line 'From: Jeff King <peff@peff.net>'
(body) Adding cc: Stable <stable@vger.kernel.org> [4.8+] from line 'Cc: Stable <stable@vger.kernel.org> [4.8+]'
Dry-OK. Log says:
Sendmail: /usr/sbin/sendmail -i peff@peff.net stable@vger.kernel.org
From: Jeff King <peff@peff.net>
To: peff@peff.net
Cc: "Stable [4.8+]" <stable@vger.kernel.org>
Subject: [PATCH] the subject
Date: Mon, 10 Oct 2016 17:44:25 -0400
Message-Id: <20161010214425.9761-1-peff@peff.net>
X-Mailer: git-send-email 2.10.1.527.g93d4615
Result: OK
So it looks like it parsed the address, and shifted the "4.8+" bit into
the name, which seems reasonable. Does my example behave differently on
your system? If not, can you see what's different between your
real-world case and the example?
It might also be related to which perl modules are available. We'll use
Mail::Address if you have it, but some fallback routines if you don't.
They may behave differently.
Alternatively, if this used to work, you might try bisecting it.
-Peff
^ permalink raw reply
* Formatting problem send_mail in version 2.10.0
From: Larry Finger @ 2016-10-10 21:00 UTC (permalink / raw)
To: git
I have recently switched to openSUSE Leap 42.2 and found that some of the
features of send_mail no longer work. The problem occurs when trying to add
information to a Cc to Stable.
The initial pass through the patch produces the output
(body) Adding cc: Stable <stable@vger.kernel.org> [4.8+] from line 'Cc: Stable
<stable@vger.kernel.org> [4.8+]'
That is correct, but the actual Cc list contains
Stable <stable@vger.kernel.org[4.8+]>,
The mangled address is not legal and the mail attempt fails.
Thanks,
Larry
^ permalink raw reply
* Re: %C(auto) not working as expected
From: Jeff King @ 2016-10-10 20:55 UTC (permalink / raw)
To: René Scharfe; +Cc: Tom Hale, git, Duy Nguyen, Junio C Hamano
In-Reply-To: <6689ae49-6095-7bb4-ea06-1aaded174811@web.de>
On Mon, Oct 10, 2016 at 10:52:47PM +0200, René Scharfe wrote:
> > I like the intent here, though I found "Values, color" hard to parse (it
> > was not immediately clear that you mean "the color paragraph of the
> > Values section", as commas are already being used in that sentence for
> > the parenthetical phrase).
> >
> > I'm not sure how to say that succinctly, as we are four levels deep
> > (git-config -> configuration file -> values -> color). Too bad there is
> > no good link syntax for it. Maybe:
> >
> > ...color specification, as described in linkgit:git-config[1] (see the
> > paragraph on colors in the "Values" section, under "CONFIGURATION
> > FILE")
> >
> > or something.
>
> That's better. Or we could remove the "color" part and trust readers to
> find the right paragraph in the Values sub-section?
>
> -- '%C(...)': color specification, as described in color.branch.* config option;
> +- '%C(...)': color specification, as described under Values in the
> + "CONFIGURATION FILE" section of linkgit:git-config[1];
Yeah, I think that is plenty of roadmap to give the user.
-Peff
^ permalink raw reply
* Re: [PATCH] pretty: respect color settings for %C placeholders
From: Jeff King @ 2016-10-10 20:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, René Scharfe, Tom Hale, git
In-Reply-To: <20161010185935.r3wnfglpi7hjjpdk@sigill.intra.peff.net>
On Mon, Oct 10, 2016 at 02:59:35PM -0400, Jeff King wrote:
> > I must be reading the patch incorrectly, but I cannot quite tell
> > where I want astray...
>
> No, we don't come here from %C() at all. This is for bare "%Cred", which
> cannot have "always" (or "auto"), as there is no syntactic spot for it.
> It is mostly historical (it _only_ supports red, green, and blue). We
> could actually leave this as-is to show the colors unconditionally. I
> changed it to keep the new behavior consistent, but I doubt anybody
> cares much either way.
Speaking of consistent behavior, if we do this, I think we should give
"%(color:red)" in for-each-ref and tag the same treatment. That requires
some infrastructure refactoring to get the value down to the
ref-formatting code. I'm working on it, but might not have it out today.
-Peff
^ permalink raw reply
* Re: %C(auto) not working as expected
From: René Scharfe @ 2016-10-10 20:52 UTC (permalink / raw)
To: Jeff King; +Cc: Tom Hale, git, Duy Nguyen, Junio C Hamano
In-Reply-To: <20161009234617.y6xfjyv6xjkf2afi@sigill.intra.peff.net>
Am 10.10.2016 um 01:46 schrieb Jeff King:
>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
>> index a942d57..89e3bc6 100644
>> --- a/Documentation/pretty-formats.txt
>> +++ b/Documentation/pretty-formats.txt
>> @@ -166,7 +166,8 @@ endif::git-rev-list[]
>> - '%Cgreen': switch color to green
>> - '%Cblue': switch color to blue
>> - '%Creset': reset color
>> -- '%C(...)': color specification, as described in color.branch.* config option;
>> +- '%C(...)': color specification, as described under Values, color in the
>> + "CONFIGURATION FILE" section of linkgit:git-config[1];
>
> I like the intent here, though I found "Values, color" hard to parse (it
> was not immediately clear that you mean "the color paragraph of the
> Values section", as commas are already being used in that sentence for
> the parenthetical phrase).
>
> I'm not sure how to say that succinctly, as we are four levels deep
> (git-config -> configuration file -> values -> color). Too bad there is
> no good link syntax for it. Maybe:
>
> ...color specification, as described in linkgit:git-config[1] (see the
> paragraph on colors in the "Values" section, under "CONFIGURATION
> FILE")
>
> or something.
That's better. Or we could remove the "color" part and trust readers to
find the right paragraph in the Values sub-section?
-- '%C(...)': color specification, as described in color.branch.* config option;
+- '%C(...)': color specification, as described under Values in the
+ "CONFIGURATION FILE" section of linkgit:git-config[1];
^ permalink raw reply
* Re: [PATCH] use strbuf_add_unique_abbrev() for adding short hashes, part 3
From: Jeff King @ 2016-10-10 20:52 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <af55f6d7-e1b1-272b-4fbe-a6eb2422b3be@web.de>
On Mon, Oct 10, 2016 at 10:46:21PM +0200, René Scharfe wrote:
> Good question. ALLOC_GROW() doesn't double exactly, but indeed the
> number of reallocations depends on the size of the added pieces. I
> always thought of strbuf_addf() as an expensive function for
> convenience, but never timed it.
>
> Numbers vary a bit, but here's what I get for the crude test program
> at the end:
>
> $ time t/helper/test-strbuf strbuf_addf 123 123456789012345678901234567890
> 123123456789012345678901234567890
>
> real 0m0.168s
> user 0m0.164s
> sys 0m0.000s
> $ time t/helper/test-strbuf strbuf_addstr 123 123456789012345678901234567890
> 123123456789012345678901234567890
>
> real 0m0.141s
> user 0m0.140s
> sys 0m0.000s
>
> Just a data-point, but it confirms my bias, so I stop here. :)
Heh. I'm surprised it's that big a difference, as processing simple
printf strings should be pretty quick. I guess what happens in your
program is that your strings almost always require a re-allocation
(because you've just released, and we start small), and we literally end
up doing a partial copy via vsnprintf(), realizing we're out of space,
reallocating, and then running it again.
So it's noticeably worse when we _do_ reallocate, but usually that
should be amortized across many calls (and if it isn't, then you are
paying the much bigger price of lots of mallocs, and you should optimize
that first :) ).
That being said, it doesn't seem like it would be _worse_ to move from
addf to multiple addstrs.
-Peff
^ permalink raw reply
* Re: How to watch a mailing list & repo for patches which affect a certain area of code?
From: Ian Kelling @ 2016-10-10 20:49 UTC (permalink / raw)
To: Stefan Beller, git, Xiaolong Ye
In-Reply-To: <CAGZ79kb2HWmaW3XpfHRj8vcOStPoQmR_NZe7RCRhw=FnnHbZ8A@mail.gmail.com>
On Mon, Oct 10, 2016, at 12:08 PM, Stefan Beller wrote:
> Well it is found in 2.9 and later. Currently the base footer is
> opt-in, e.g. you'd
> need to convince people to run `git config format.useAutoBase true` or to
> manually add the base to the patch via `format-patch --base=<commit>`.
Nice. Another useful config option this lead me to find is git config
--global branch.autoSetupMerge always which sets up the remote for local
branches, allowing useAutoBase to work for them without extra typing
(according to the man page, I haven't tried it yet).
^ permalink raw reply
* Re: [PATCH] contrib: add credential helper for libsecret
From: Jeff King @ 2016-10-10 20:46 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: Mantas Mikulėnas, git
In-Reply-To: <1476130850.7457.8.camel@kaarsemaker.net>
On Mon, Oct 10, 2016 at 10:20:50PM +0200, Dennis Kaarsemaker wrote:
> On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:
> > This is based on the existing gnome-keyring helper, but instead of
> > libgnome-keyring (which was specific to GNOME and is deprecated), it
> > uses libsecret which can support other implementations of XDG Secret
> > Service API.
> >
> > Passes t0303-credential-external.sh.
>
> When setting credential.helper to this helper, I get the following output:
>
> $ git clone https://private-repo-url-removed private
> Cloning into 'private'...
> /home/dennis/code/git/contrib/credential/libsecret/ get: 1: /home/dennis/code/git/contrib/credential/libsecret/ get: /home/dennis/code/git/contrib/credential/libsecret/: Permission denied
>
> Looks suboptimal. Am I holding it wrong?
That looks like a directory name in your error message. How did you set
up credential.helper? I'd expect normal usage to be something like this:
# do this once, or cp the binary into your $PATH
PATH=$PATH:/home/dennis/code/git/contrib/credential/libsecret
git config --global credential.helper libsecret
But if you don't want to put it in your PATH, then I think:
git config --global credential.helper \
'!/home/dennis/code/git/contrib/credential/git-credential-libsecret'
would work.
-Peff
^ permalink raw reply
* Re: [PATCH] use strbuf_add_unique_abbrev() for adding short hashes, part 3
From: René Scharfe @ 2016-10-10 20:46 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano
In-Reply-To: <20161010000035.mfcf55wqfcbcnarh@sigill.intra.peff.net>
Am 10.10.2016 um 02:00 schrieb Jeff King:
> On Sat, Oct 08, 2016 at 05:38:47PM +0200, René Scharfe wrote:
>
>> Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs
>> instead of taking detours through find_unique_abbrev() and its static
>> buffer. This is shorter in most cases and a bit more efficient.
>>
>> The changes here are not easily handled by a semantic patch because
>> they involve removing temporary variables and deconstructing format
>> strings for strbuf_addf().
>
> Yeah, the thing to look for here is whether the sha1 variable holds the
> same value at both times.
>
> These all look OK to me. Mild rambling below.
>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index 5200d5c..9041c2f 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -202,9 +202,9 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)
>> strbuf_addf(&o->obuf, "virtual %s\n",
>> merge_remote_util(commit)->name);
>> else {
>> - strbuf_addf(&o->obuf, "%s ",
>> - find_unique_abbrev(commit->object.oid.hash,
>> - DEFAULT_ABBREV));
>> + strbuf_add_unique_abbrev(&o->obuf, commit->object.oid.hash,
>> + DEFAULT_ABBREV);
>> + strbuf_addch(&o->obuf, ' ');
>
> I've often wondered whether a big strbuf_addf() is more efficient than
> several strbuf_addstrs. It amortizes the size-checks, certainly, though
> those are probably not very big. It shouldn't matter much for amortizing
> the cost of malloc, as it's very unlikely to have a case where:
>
> strbuf_addf("%s%s", foo, bar);
>
> would require one malloc, but:
>
> strbuf_addstr(foo);
> strbuf_addstr(bar);
>
> would require two (one of the strings would have to be around the same
> size as the ALLOC_GROW() doubling).
>
> So it probably doesn't matter much in practice (not that most of these
> cases are very performance sensitive anyway). Mostly just something I've
> pondered while tweaking strbuf invocations.
Good question. ALLOC_GROW() doesn't double exactly, but indeed the
number of reallocations depends on the size of the added pieces. I
always thought of strbuf_addf() as an expensive function for
convenience, but never timed it.
Numbers vary a bit, but here's what I get for the crude test program
at the end:
$ time t/helper/test-strbuf strbuf_addf 123 123456789012345678901234567890
123123456789012345678901234567890
real 0m0.168s
user 0m0.164s
sys 0m0.000s
$ time t/helper/test-strbuf strbuf_addstr 123 123456789012345678901234567890
123123456789012345678901234567890
real 0m0.141s
user 0m0.140s
sys 0m0.000s
Just a data-point, but it confirms my bias, so I stop here. :)
> Just thinking aloud, I've also wondered if strbuf_addoid() would be
> handy. We already have oid_to_hex_r(). In fact, you could write it
> already as:
>
> strbuf_add_unique_abbrev(sb, oidp->hash, 0);
>
> but that is probably not helping maintainability. ;)
Yes, a function for adding full hashes without using a static
variable is useful for library functions that may end up being
called often or in parallel. I'd call it strbuf_add_hash,
though.
diff --git a/Makefile b/Makefile
index 1aad150..ad5e98c 100644
--- a/Makefile
+++ b/Makefile
@@ -628,6 +628,7 @@ TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
TEST_PROGRAMS_NEED_X += test-sha1
TEST_PROGRAMS_NEED_X += test-sha1-array
TEST_PROGRAMS_NEED_X += test-sigchain
+TEST_PROGRAMS_NEED_X += test-strbuf
TEST_PROGRAMS_NEED_X += test-string-list
TEST_PROGRAMS_NEED_X += test-submodule-config
TEST_PROGRAMS_NEED_X += test-subprocess
diff --git a/t/helper/test-strbuf.c b/t/helper/test-strbuf.c
new file mode 100644
index 0000000..177f3e2
--- /dev/null
+++ b/t/helper/test-strbuf.c
@@ -0,0 +1,25 @@
+#include "cache.h"
+
+int cmd_main(int argc, const char **argv)
+{
+ struct strbuf sb = STRBUF_INIT;
+ int i = 1000000;
+
+ if (argc == 4) {
+ if (!strcmp(argv[1], "strbuf_addf")) {
+ while (i--) {
+ strbuf_release(&sb);
+ strbuf_addf(&sb, "%s%s", argv[2], argv[3]);
+ }
+ }
+ if (!strcmp(argv[1], "strbuf_addstr")) {
+ while (i--) {
+ strbuf_release(&sb);
+ strbuf_addstr(&sb, argv[2]);
+ strbuf_addstr(&sb, argv[3]);
+ }
+ }
+ puts(sb.buf);
+ }
+ return 0;
+}
^ permalink raw reply related
* Re: [PATCH] contrib: add credential helper for libsecret
From: Dennis Kaarsemaker @ 2016-10-10 20:20 UTC (permalink / raw)
To: Mantas Mikulėnas, git
In-Reply-To: <20161009123417.147239-1-grawity@gmail.com>
On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:
> This is based on the existing gnome-keyring helper, but instead of
> libgnome-keyring (which was specific to GNOME and is deprecated), it
> uses libsecret which can support other implementations of XDG Secret
> Service API.
>
> Passes t0303-credential-external.sh.
When setting credential.helper to this helper, I get the following output:
$ git clone https://private-repo-url-removed private
Cloning into 'private'...
/home/dennis/code/git/contrib/credential/libsecret/ get: 1: /home/dennis/code/git/contrib/credential/libsecret/ get: /home/dennis/code/git/contrib/credential/libsecret/: Permission denied
Looks suboptimal. Am I holding it wrong?
D.
^ permalink raw reply
* Re: [PATCH v1 0/2] convert: stream and early out
From: Junio C Hamano @ 2016-10-10 20:19 UTC (permalink / raw)
To: tboegi; +Cc: git
In-Reply-To: <20161009095649.1886-1-tboegi@web.de>
tboegi@web.de writes:
> From: Torsten Bögershausen <tboegi@web.de>
>
> An optimization when autocrlf is used and the binary/text detection is run.
> Or git ls-files --eol is run to analyze the content of files or blobs.
This looks like a worthwhile thing to do. Please sign-off the
patches when they are finalized.
Thanks.
>
> Torsten Bögershausen (2):
> read-cache: factor out get_sha1_from_index() helper
> convert.c: stream and early out
>
> cache.h | 3 +
> convert.c | 195 +++++++++++++++++++++++++++++++++++++++--------------------
> read-cache.c | 29 +++++----
> 3 files changed, 151 insertions(+), 76 deletions(-)
^ permalink raw reply
* Re: [PATCH v1 2/2] convert.c: stream and early out
From: Junio C Hamano @ 2016-10-10 20:19 UTC (permalink / raw)
To: tboegi; +Cc: git
In-Reply-To: <20161009095654.1964-1-tboegi@web.de>
tboegi@web.de writes:
> -static void gather_stats(const char *buf, unsigned long size, struct text_stat *stats)
> +static void gather_stats_partly(const char *buf, unsigned long len,
> + struct text_stat *stats, unsigned earlyout)
> {
I think it is OK not to rename the function (you'd be passing earlyout=0
for callers that want exact stat, right?).
> unsigned long i;
>
> - memset(stats, 0, sizeof(*stats));
> -
> - for (i = 0; i < size; i++) {
> + if (!buf || !len)
> + return;
> + for (i = 0; i < len; i++) {
> unsigned char c = buf[i];
> if (c == '\r') {
> - if (i+1 < size && buf[i+1] == '\n') {
> + stats->stat_bits |= CONVERT_STAT_BITS_ANY_CR;
> + if (i+1 < len && buf[i+1] == '\n') {
> stats->crlf++;
> i++;
> - } else
> + stats->stat_bits |= CONVERT_STAT_BITS_TXT_CRLF;
> + } else {
> stats->lonecr++;
> + stats->stat_bits |= CONVERT_STAT_BITS_BIN;
> + }
> continue;
> }
> if (c == '\n') {
> stats->lonelf++;
> + stats->stat_bits |= CONVERT_STAT_BITS_TXT_LF;
> continue;
> }
> if (c == 127)
> @@ -67,7 +74,7 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *
> stats->printable++;
> break;
> case 0:
> - stats->nul++;
> + stats->stat_bits |= CONVERT_STAT_BITS_BIN;
> /* fall through */
> default:
> stats->nonprintable++;
So depending on the distribution of the bytes in the file, the
bitfields in stats->stat_bits will be filled one bit at a time in
random order.
> @@ -75,10 +82,12 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *
> }
> else
> stats->printable++;
> + if (stats->stat_bits & earlyout)
> + break; /* We found what we have been searching for */
But an "earlyout" says that if "any" of the earlyout bit is seen, we
can return.
It somehow felt a bit too limited to me in my initial reading, but I
guess I shouldn't be surprised to see that such a limited interface
is sufficient for a file-local helper function ;-).
The only caller that the semantics of this exit condition matters is
the one that wants to know "do we have NUL or CR anywhere?", so I
guess this should be sufficient.
> }
>
> /* If file ends with EOF then don't count this EOF as non-printable. */
> - if (size >= 1 && buf[size-1] == '\032')
> + if (len >= 1 && buf[len-1] == '\032')
> stats->nonprintable--;
This noise is somewhat irritating. Was there a reason why size was
a bad name for the variable?
> +static const char *convert_stats_ascii(unsigned convert_stats)
> {
> - unsigned int convert_stats = gather_convert_stats(data, size);
> -
> + const unsigned mask = CONVERT_STAT_BITS_TXT_LF |
> + CONVERT_STAT_BITS_TXT_CRLF;
> if (convert_stats & CONVERT_STAT_BITS_BIN)
> return "-text";
> - switch (convert_stats) {
> + switch (convert_stats & mask) {
> case CONVERT_STAT_BITS_TXT_LF:
> return "lf";
> case CONVERT_STAT_BITS_TXT_CRLF:
Subtle. The caller runs the stat colllection with early-out set to
BITS_BIN, so that this can set "-text" early. It knows that without
BITS_BIN, the stat was taken for the whole contents and the check lf
or crlf can be reliable.
I wonder if we can/need to do something to remove this subtleness
out of this callchain, which could be a source of confusion.
> @@ -132,24 +162,45 @@ static const char *gather_convert_stats_ascii(const char *data, unsigned long si
> }
> }
>
> +static unsigned get_convert_stats_wt(const char *path)
> +{
> + struct text_stat stats;
> + unsigned earlyout = CONVERT_STAT_BITS_BIN;
> + int fd;
> + memset(&stats, 0, sizeof(stats));
> + fd = open(path, O_RDONLY);
> + if (fd < 0)
> + return 0;
> + for (;;) {
> + char buf[2*1024];
Where is this 2kB come from? Out of thin air?
^ permalink raw reply
* Re: [PATCH] pretty: respect color settings for %C placeholders
From: Jeff King @ 2016-10-10 20:04 UTC (permalink / raw)
To: René Scharfe; +Cc: Duy Nguyen, Tom Hale, git, Junio C Hamano
In-Reply-To: <19e59db7-f3dd-35ec-8cf1-b070b1c05abe@web.de>
On Mon, Oct 10, 2016 at 09:59:17PM +0200, René Scharfe wrote:
> > > Shouldn't we have an "else" here?
> >
> > I'm not sure what you mean; can you write it out?
>
> > - if (skip_prefix(begin, "auto,", &begin)) {
> > +
> > + if (!skip_prefix(begin, "always,", &begin)) {
> > if (!want_color(c->pretty_ctx->color))
> > return end - placeholder + 1;
> > }
>
> else { /* here */
>
> > + /* this is a historical noop */
> > + skip_prefix(begin, "auto,", &begin);
>
> }
>
> Otherwise "always,auto," would be allowed and mean the same as "always,",
> which just seems wrong. Not a biggie.
I don't think that will parse "%C(auto,foo)", as we hit the
!skip_prefix() of the conditional, and do not look for "auto," at all.
I think you'd have to move the check for "auto," inside the if block.
I'm leaning towards just writing it out the long way, though, as I did
in my reply to Junio.
> > > Perhaps it's a funtion like add_color(sb, ctx, color) or similar would be
> > > nice?
> >
> > I actually wrote it that way first (I called it "maybe_add_color()"),
> > but it felt a little funny to have a separate function that people might
> > be tempted to reuse (the right solution is generally to check
> > want_color() early as above, but we can't do that here because we have
> > to find the end of each placeholder).
>
> OK. A variable then? Lazy pseudo-code:
>
> if (RED)
> color = red;
> else if (GREEN)
> ...
>
> if (want_color())
> strbuf_addstr(sb, color);
Yeah, that is a bit more clear (the final conditional just needs to
check that we actually found a color).
-Peff
^ permalink raw reply
* Re: [PATCH] pretty: respect color settings for %C placeholders
From: René Scharfe @ 2016-10-10 19:59 UTC (permalink / raw)
To: Jeff King; +Cc: Duy Nguyen, Tom Hale, git, Junio C Hamano
In-Reply-To: <20161010174257.b4uxplavefjyr6rl@sigill.intra.peff.net>
Am 10.10.2016 um 19:42 schrieb Jeff King:
> On Mon, Oct 10, 2016 at 07:09:12PM +0200, René Scharfe wrote:
>
>>> diff --git a/pretty.c b/pretty.c
>>> index 25efbca..73e58b5 100644
>>> --- a/pretty.c
>>> +++ b/pretty.c
>>> @@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
>>>
>>> if (!end)
>>> return 0;
>>> - if (skip_prefix(begin, "auto,", &begin)) {
>>> +
>>> + if (!skip_prefix(begin, "always,", &begin)) {
>>> if (!want_color(c->pretty_ctx->color))
>>> return end - placeholder + 1;
>>> }
>>
>> Shouldn't we have an "else" here?
>
> I'm not sure what you mean; can you write it out?
> - if (skip_prefix(begin, "auto,", &begin)) {
> +
> + if (!skip_prefix(begin, "always,", &begin)) {
> if (!want_color(c->pretty_ctx->color))
> return end - placeholder + 1;
> }
else { /* here */
> + /* this is a historical noop */
> + skip_prefix(begin, "auto,", &begin);
}
Otherwise "always,auto," would be allowed and mean the same as
"always,", which just seems wrong. Not a biggie.
>>> - if (skip_prefix(placeholder + 1, "red", &rest))
>>> + if (skip_prefix(placeholder + 1, "red", &rest) &&
>>> + want_color(c->pretty_ctx->color))
>>> strbuf_addstr(sb, GIT_COLOR_RED);
>>> - else if (skip_prefix(placeholder + 1, "green", &rest))
>>> + else if (skip_prefix(placeholder + 1, "green", &rest) &&
>>> + want_color(c->pretty_ctx->color))
>>> strbuf_addstr(sb, GIT_COLOR_GREEN);
>>> - else if (skip_prefix(placeholder + 1, "blue", &rest))
>>> + else if (skip_prefix(placeholder + 1, "blue", &rest) &&
>>> + want_color(c->pretty_ctx->color))
>>> strbuf_addstr(sb, GIT_COLOR_BLUE);
>>> - else if (skip_prefix(placeholder + 1, "reset", &rest))
>>> + else if (skip_prefix(placeholder + 1, "reset", &rest) &&
>>> + want_color(c->pretty_ctx->color))
>>> strbuf_addstr(sb, GIT_COLOR_RESET);
>>> return rest - placeholder;
>>> }
>>
>> Perhaps it's a funtion like add_color(sb, ctx, color) or similar would be
>> nice?
>
> I actually wrote it that way first (I called it "maybe_add_color()"),
> but it felt a little funny to have a separate function that people might
> be tempted to reuse (the right solution is generally to check
> want_color() early as above, but we can't do that here because we have
> to find the end of each placeholder).
OK. A variable then? Lazy pseudo-code:
if (RED)
color = red;
else if (GREEN)
...
if (want_color())
strbuf_addstr(sb, color);
> What I have here is a little funny, too, though, as it keeps trying
> other color names if it finds "red" but want_color() returns 0.
Oh, missed that somehow.. O_o
^ permalink raw reply
* Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL
From: Dennis Kaarsemaker @ 2016-10-10 19:58 UTC (permalink / raw)
To: Stefan Beller, gitster; +Cc: git, venv21
In-Reply-To: <20161010175611.1058-1-sbeller@google.com>
[And now with CC to the list, sorry Stefan]
On Mon, 2016-10-10 at 10:56 -0700, Stefan Beller wrote:
> Before 63e95beb0 (2016-04-15, submodule: port resolve_relative_url from
> shell to C), it did not matter if the superprojects URL had a trailing
> slash or not. It was just chopped off as one of the first steps
> (The "remoteurl=${remoteurl%/}" near the beginning of
> resolve_relative_url(), which was removed in said commit).
>
> When porting this to the C version, an off-by-one error was introduced
> and we did not check the actual last character to be a slash, but the
> NULL delimiter.
>
> Reintroduce the behavior from before 63e95beb0, to ignore the trailing
> slash.
Looks good to me, and fixes my simple testcase and cloning epiphany
with trailing slash. Thanks!
D.
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Junio C Hamano @ 2016-10-10 19:58 UTC (permalink / raw)
To: larsxschneider; +Cc: git, jnareb, peff
In-Reply-To: <20161008112530.15506-14-larsxschneider@gmail.com>
larsxschneider@gmail.com writes:
> +# Count unique lines in two files and compare them.
> +test_cmp_count () {
> + for FILE in $@
> + do
> + sort $FILE | uniq -c | sed "s/^[ ]*//" >$FILE.tmp
> + cat $FILE.tmp >$FILE
Unquoted references to $FILE bothers me. Are you relying on them
getting split at IFS boundaries? Otherwise write this (and other
similar ones) like so:
for FILE in "$@"
do
do-this-to "$FILE" | ... >"$FILE.tmp" &&
cat "$FILE.tmp" >"$FILE" &&
rm -f "$FILE.tmp"
> + done &&
> + test_cmp $@
The use of "$@" here is quite pointless, as you _know_ all of them
are filenames, and you _know_ that test_cmp takes only two
filenames. Be explicit and say
test_cmp "$1" "$2"
or even
test_cmp_count () {
expect=$1 actual=$2
for FILE in "$expect" "$actual"
do
...
done &&
test_cmp "$expect" "$actual"
> +# Count unique lines except clean invocations in two files and compare
> +# them. Clean invocations are not counted because their number can vary.
> +# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
> +test_cmp_count_except_clean () {
> + for FILE in $@
> + do
> + sort $FILE | uniq -c | sed "s/^[ ]*//" |
> + sed "s/^\([0-9]\) IN: clean/x IN: clean/" >$FILE.tmp
> + cat $FILE.tmp >$FILE
> + done &&
> + test_cmp $@
> +}
Why do you even _care_ about the number of invocations? While I
told you why "clean" could be called multiple times under racy Git
as an example, that was not meant to be an exhaustive example. I
wouldn't be surprised if we needed to run smudge twice, for example,
in some weirdly racy cases in the future.
Can we just have the correctness (i.e. "we expect that the working
tree file gets this as the result of checking it out, and we made
sure that is the case") test without getting into such an
implementation detail?
Thanks.
^ permalink raw reply
* [PATCHv2] documentation: improve submodule.<name>.{url, path} description
From: Stefan Beller @ 2016-10-10 19:36 UTC (permalink / raw)
To: gitster; +Cc: git, Jens.Lehmann, hvoigt, Stefan Beller
In-Reply-To: <xmqqshs4dnq1.fsf@gitster.mtv.corp.google.com>
Unlike the url variable a user cannot override the the path variable,
as it is part of the content together with the gitlink at the given
path. To avoid confusion do not mention the .path variable in the config
section and rely on the documentation provided in gitmodules[5].
Enhance the description of submodule.<name>.url and mention its two use
cases separately.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
I think the gitmodules[5] is enough for .path, so let's just
do this one instead.
Thanks,
Stefan
Documentation/config.txt | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index e78293b..fd775b4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2811,12 +2811,13 @@ stash.showStat::
option will show diffstat of the stash. Defaults to true.
See description of 'show' command in linkgit:git-stash[1].
-submodule.<name>.path::
submodule.<name>.url::
- The path within this project and URL for a submodule. These
- variables are initially populated by 'git submodule init'. See
- linkgit:git-submodule[1] and linkgit:gitmodules[5] for
- details.
+ The URL for a submodule. This variable is copied from the .gitmodules
+ file to the git config via 'git submodule init'. The user can change
+ the configured URL before obtaining the submodule via 'git submodule
+ update'. After obtaining the submodule, the presence of this variable
+ is used as a sign whether the submodule is of interest to git commands.
+ See linkgit:git-submodule[1] and linkgit:gitmodules[5] for details.
submodule.<name>.update::
The default update procedure for a submodule. This variable
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* RE: How to watch a mailing list & repo for patches which affect a certain area of code? [OT]
From: Jason Pyeron @ 2016-10-10 19:40 UTC (permalink / raw)
To: git; +Cc: 'Ian Kelling', 'Xiaolong Ye',
'Stefan Beller'
In-Reply-To: <CAGZ79kb2HWmaW3XpfHRj8vcOStPoQmR_NZe7RCRhw=FnnHbZ8A@mail.gmail.com>
> -----Original Message-----
> From: Stefan Beller
> Sent: Monday, October 10, 2016 15:08
>
> On Mon, Oct 10, 2016 at 11:56 AM, Jason Pyeron wrote:
> >> -----Original Message-----
> >> From: Stefan Beller
> >> Sent: Monday, October 10, 2016 14:43
> >>
> >> +cc Xiaolong Ye <xiaolong.ye@intel.com>
> >>
> >> On Sun, Oct 9, 2016 at 2:26 PM, Jason Pyeron wrote:
> >> >> -----Original Message-----
> >> >> From: Ian Kelling
> >> >> Sent: Sunday, October 09, 2016 15:03
> >> >>
> >> >> I've got patches in various projects, and I don't have
> >> time to keep up
> >> >> with the mailing list, but I'd like to help out with
> >> >> maintenance of that
> >> >> code, or the functions/files it touches. People don't cc me.
> >> >> I figure I
> >> >> could filter the list, test patches submitted, commits made,
> >> >> mentions of
> >> >> files/functions, build filters based on the code I have in
> >> >> the repo even
> >> >> if it's been moved or changed subsequently. I'm wondering
> >> what other
> >> >> people have implemented already for automation around
> >> this, or general
> >> >> thoughts. Web search is not showing me much.
> >> >>
> >> >
> >> > One thought would be to apply every patch automatically (to
> >> the branches of interest?). Then trigger on the
> [successful] changed
> >> > code. This would simplify the logic to working on the
> >> source only and not parsing the emails.
> >> >
> >> > -Jason
> >> >
> >>
> >> I think this is currently attempted by some kernel people.
> >> However it is very hard to tell where to apply a patch, as it
> >
> > This is one of the reasons why I use bundles instead of
> format patch.
>
> Oh! That sounds interesting for solving the problem where to apply
> a change, but the big disadvantage of bundles to patches is
> the inability
> to just comment on it with an inline response.
Yep. It is a big one. I have a personal project to add a footer to a format patch with the missing "binary" data. The thoughts were for the main cases using a RLE bitmap for the whitespace in the above patch and the remainder of the commit blob data. This would allow minimal duplicate information in the email but pure text changes would be binary perfect so the commit id will still be correct.
Sigh, never have enough free time.
> So I assume you follow
> a different workflow than git or the kernel do. Which project
> do you use it
> for and do you have some documentation/blog that explains
> that workflow?
This is used when collaborating cross enterprise. In these situations enterprise A will not give access to enterprise B on their CI system, or git repo, etc...
We all have a mailing list in common (encrypted/signed emails when it contains sensitive info). The rules prevent us from using cloud solutions for almost all of our work.
I have also worked on git for cross domain (tin foil hat time) source code transfer.
As to a blog, never thought about it. Ask questions on the list and I will help.
>
>
> >
> >> is not formalized.
> >> See the series that was merged at 72ce3ff7b51c
> >> ('xy/format-patch-base'),
> >> which adds a footer to the patch, that tells you where
> >> exactly a patch ought
> >> to be applied.
> >
> > Cant wait for that.
>
> Well it is found in 2.9 and later. Currently the base footer is
> opt-in, e.g. you'd
> need to convince people to run `git config format.useAutoBase
> true` or to
> manually add the base to the patch via `format-patch --base=<commit>`.
Please make default in 2.10.2 .
>
> >
> >>
> >> The intention behind that series was to have some CI
> system hooked up
> >> and report failures to the mailing list as well IIUC. Maybe
> >> that helps with
> >> your use case, too?
> >
> > I envisioned that it would try for each head he was interested in.
> >
>
> Well the test system can be smart enough to differentiate between:
For the OP's case
Test 1: does it cleanly apply to any head, if no for all raise flag.
> * the patch you sent did not even compile on your base, so why
> are you sending bogus patches?
Test 2: is it in an area I care about, if not stop.
Test 3: does it compile for clean application, if no for all raise flag.
> * the patch you sent was fine as you sent it, but in the mean time
> the target head progressed, and it doesn't compile/test any more.
> collaboration is hard.
Yes, especially when you have no time, or management is in the way.
> * or an extension to the prior point: this patch is fine but is broken
> by the series xyz that is also in flight, please coordinate with
> name@email.
^ permalink raw reply
* Re: git 2.10.1 test regression in t3700-add.sh
From: Johannes Sixt @ 2016-10-10 19:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeremy Huddleston Sequoia, t.gummerer, git
In-Reply-To: <xmqq8ttwgkyo.fsf@gitster.mtv.corp.google.com>
Am 10.10.2016 um 19:41 schrieb Junio C Hamano:
> I also notice that the problematic test uses "chmod 755"; don't we
> need POSIXPERM prerequisite on this test, too, I wonder?
Good point. Without POSIXPERM the test demonstrate that, since chmod 755
is basically a noop, the following add --chmod=-x does not leave an x
bit in the index when there was none there in the first place. I think
it does not hurt to keep the test even though it does not quite test the
same thing as on POSIXPERM enabled systems.
-- Hannes
^ 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