* Re: [PATCH] clean up confusing suggestion for commit references
From: Heiko Voigt @ 2016-10-11 16:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqoa2sdnjt.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 10, 2016 at 12:14:14PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Mon, Oct 10, 2016 at 11:24:01AM -0700, Junio C Hamano wrote:
> >
> >> I no longer have preference either way myself, even though I was in
> >> favor of no-quotes simply because I had an alias to produce that
> >> format and was used to it.
> >
> > I'll admit that I don't care _that_ much and am happy to leave it up to
> > individual authors, as long as nobody quotes SubmittingPatches at me as
> > some kind of gospel when I use the no-quotes form.
>
> ;-).
>
> I just do not want to hear "gitk (or was it git-gui) produces quoted
> form, why are you recommending no-quoted form in SubmittingPatches?"
>
> I'd say "use common sense; sometimes it is less confusing to read
> without quotes and it is perfectly OK to do so if that is the case".
I do not care about which format it should be either. I just wanted to
be clear about whatever should be used. Since it seems we will allow
both, I am also fine with leaving the description as it is ;-)
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 1/2] submodule add: extend force flag to add existing repos
From: Heiko Voigt @ 2016-10-11 16:36 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, git@vger.kernel.org, Linus Torvalds, Jeff King
In-Reply-To: <CAGZ79kbg1KaeL14DMcV=ObaZOBpJpxOZAEYY1mmf8eqDUUqYmA@mail.gmail.com>
Hi,
On Fri, Oct 07, 2016 at 10:25:04AM -0700, Stefan Beller wrote:
> On Fri, Oct 7, 2016 at 5:52 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > On Thu, Oct 06, 2016 at 01:11:20PM -0700, Junio C Hamano wrote:
> >> Stefan Beller <sbeller@google.com> writes:
> >>
> >> > Currently the force flag in `git submodule add` takes care of possibly
> >> > ignored files or when a name collision occurs.
> >> >
> >> > However there is another situation where submodule add comes in handy:
> >> > When you already have a gitlink recorded, but no configuration was
> >> > done (i.e. no .gitmodules file nor any entry in .git/config) and you
> >> > want to generate these config entries. For this situation allow
> >> > `git submodule add` to proceed if there is already a submodule at the
> >> > given path in the index.
> >
> > Is it important that the submodule is in the index?
>
> If it is not in the index, it already works.
Ah ok I was not aware of that, sorry.
> > How about worktree?
> > From the index entry alone we can not deduce the values anyway.
>
> Right, but as of now this is the only show stopper, i.e.
> * you have an existing repo? -> fine, it works with --force
> * you even ignored that repo -> --force knows how to do it.
> * you already have a gitlink -> Sorry, you're out of luck.
>
> So that is why I stressed index in this commit message, as it is only about this
> case.
Forget what I wrote. As said above I was not aware that there is only an
error when it is already in the index.
> > [1] http://public-inbox.org/git/%3C20160916141143.GA47240@book.hvoigt.net%3E/
>
> Current situation:
>
> > clone the submodule into a directory
> > git submodule add --force
> > git commit everything
>
> works fine, but:
>
> > clone the submodule into a directory
> > git add <gitlink>
> > git commit <gitlink> -m "Add submodule"
> > # me: "Oh crap! I did forget the configuration."
> > git submodule add --force <url> <gitlink>
> > # Git: "It already exists in the index, I am not going to produce the config for you."
>
> The last step is changed with this patch, as
> it will just work fine then.
Thanks.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH v3 05/25] sequencer: eventually release memory allocated for the option values
From: Junio C Hamano @ 2016-10-11 16:30 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqqzimbc0gw.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> 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 ...
> ... 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.
After sleeping on it, I do think this is an overkill. The pattern I
would expect for the most normal (boring) code to use is rather:
struct app_specific app;
const char *opt_x = NULL;
struct option options[] = {
...
OPT_STRING(0, "xopt", &opt_x, N_("x option"), ...),
...
OPT_END()
};
parse_options(ac, av, prefix, options, ...);
app.x_field = xstrdup_or_null(opt_x);
... other values set to app's field based on
... not just command line options but from
... other sources.
The only reason why the OPT_STRDUP appeared convenient was because
options[] element happened to use a field in the structure directly.
The patch under discussion does an equivalent of
app.x_field = xstrdup_or_null(opt_x);
but the "opt_x" happens to be the same "app.x_field" in this case,
so in that sense, it follows the normal and boring pattern.
The "struct app_specific" may not even exist in the same scope as
the caller of parse_options(), but may have to be initialized in a
function that is three-level deep in the callchain, with opt_x
variable passed through as a parameter. So OPT_STRDUP may not be a
bad or horrible idea, but it is not such a great one, either.
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Matthieu Moy @ 2016-10-11 16:18 UTC (permalink / raw)
To: Larry Finger
Cc: Jeff King, Mathieu Lienard--Mayor, Jorge Juan Garcia Garcia,
Remi Lespinet, git
In-Reply-To: <45cfc4e5-c30e-19cb-ec3e-407ceb4e3ad5@lwfinger.net>
Larry Finger <Larry.Finger@lwfinger.net> writes:
> That added information at the end is intended to be passed on to the
> stable group. In this case, the patch needs to be applied to kernel
> versions 4.8 and later.
OK, but where do people fetch this information from?
When you use git send-email, the content of the Cc: trailers ends up
both in the body of the message and in the Cc: field of the same
message.
If you need the mention to appear in the body of the message, then using
parenthesis is fine: git send-email won't remove it (more precisely,
"send-email" will call "format-patch" which won't remove it).
Not an objection to patching send-email anyway, but if there's a simple
and RFC-compliant way to do what you're looking for, we can as well use
it (possibly in addition to patching).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* [PATCH 2/2] reset: support the --stdin option
From: Johannes Schindelin @ 2016-10-11 16:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1476202100.git.johannes.schindelin@gmx.de>
Just like with other Git commands, this option makes it read the paths
from the standard input. It comes in handy when resetting many, many
paths at once and wildcards are not an option (e.g. when the paths are
generated by a tool).
Note: to keep things simple, we first parse the entire list and perform
the actual reset action only in a second phase.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/git-reset.txt | 10 +++++++-
builtin/reset.c | 56 +++++++++++++++++++++++++++++++++++++++++++--
t/t7107-reset-stdin.sh | 33 ++++++++++++++++++++++++++
3 files changed, 96 insertions(+), 3 deletions(-)
create mode 100755 t/t7107-reset-stdin.sh
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 25432d9..533ef69 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -8,7 +8,7 @@ git-reset - Reset current HEAD to the specified state
SYNOPSIS
--------
[verse]
-'git reset' [-q] [<tree-ish>] [--] <paths>...
+'git reset' [-q] [--stdin [-z]] [<tree-ish>] [--] <paths>...
'git reset' (--patch | -p) [<tree-ish>] [--] [<paths>...]
'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] [<commit>]
@@ -97,6 +97,14 @@ OPTIONS
--quiet::
Be quiet, only report errors.
+--stdin::
+ Instead of taking list of paths from the command line,
+ read list of paths from the standard input. Paths are
+ separated by LF (i.e. one path per line) by default.
+
+-z::
+ Only meaningful with `--stdin`; paths are separated with
+ NUL character instead of LF.
EXAMPLES
--------
diff --git a/builtin/reset.c b/builtin/reset.c
index c04ac07..018735f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -21,10 +21,12 @@
#include "parse-options.h"
#include "unpack-trees.h"
#include "cache-tree.h"
+#include "strbuf.h"
+#include "quote.h"
static const char * const git_reset_usage[] = {
N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
- N_("git reset [-q] [<tree-ish>] [--] <paths>..."),
+ N_("git reset [-q] [--stdin [-z]] [<tree-ish>] [--] <paths>..."),
N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),
NULL
};
@@ -267,7 +269,9 @@ static int reset_refs(const char *rev, const struct object_id *oid)
int cmd_reset(int argc, const char **argv, const char *prefix)
{
int reset_type = NONE, update_ref_status = 0, quiet = 0;
- int patch_mode = 0, unborn;
+ int patch_mode = 0, nul_term_line = 0, read_from_stdin = 0, unborn;
+ char **stdin_paths = NULL;
+ int stdin_nr = 0, stdin_alloc = 0;
const char *rev;
struct object_id oid;
struct pathspec pathspec;
@@ -286,6 +290,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
OPT_BOOL('N', "intent-to-add", &intent_to_add,
N_("record only the fact that removed paths will be added later")),
+ OPT_BOOL('z', NULL, &nul_term_line,
+ N_("paths are separated with NUL character")),
+ OPT_BOOL(0, "stdin", &read_from_stdin,
+ N_("read paths from <stdin>")),
OPT_END()
};
@@ -295,6 +303,44 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
PARSE_OPT_KEEP_DASHDASH);
parse_args(&pathspec, argv, prefix, patch_mode, &rev);
+ if (read_from_stdin) {
+ strbuf_getline_fn getline_fn = nul_term_line ?
+ strbuf_getline_nul : strbuf_getline_lf;
+ int flags = PATHSPEC_PREFER_FULL |
+ PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP;
+ struct strbuf buf = STRBUF_INIT;
+ struct strbuf unquoted = STRBUF_INIT;
+
+ if (patch_mode)
+ die(_("--stdin is incompatible with --patch"));
+
+ if (pathspec.nr)
+ die(_("--stdin is incompatible with path arguments"));
+
+ if (patch_mode)
+ flags |= PATHSPEC_PREFIX_ORIGIN;
+
+ while (getline_fn(&buf, stdin) != EOF) {
+ if (!nul_term_line && buf.buf[0] == '"') {
+ strbuf_reset(&unquoted);
+ if (unquote_c_style(&unquoted, buf.buf, NULL))
+ die(_("line is badly quoted"));
+ strbuf_swap(&buf, &unquoted);
+ }
+ ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
+ stdin_paths[stdin_nr++] = xstrdup(buf.buf);
+ strbuf_reset(&buf);
+ }
+ strbuf_release(&unquoted);
+ strbuf_release(&buf);
+
+ ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
+ stdin_paths[stdin_nr++] = NULL;
+ parse_pathspec(&pathspec, 0, flags, prefix,
+ (const char **)stdin_paths);
+ } else if (nul_term_line)
+ die(_("-z requires --stdin"));
+
unborn = !strcmp(rev, "HEAD") && get_sha1("HEAD", oid.hash);
if (unborn) {
/* reset on unborn branch: treat as reset to empty tree */
@@ -385,5 +431,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (!pathspec.nr)
remove_branch_state();
+ if (stdin_paths) {
+ while (stdin_nr)
+ free(stdin_paths[--stdin_nr]);
+ free(stdin_paths);
+ }
+
return update_ref_status;
}
diff --git a/t/t7107-reset-stdin.sh b/t/t7107-reset-stdin.sh
new file mode 100755
index 0000000..997dc42
--- /dev/null
+++ b/t/t7107-reset-stdin.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='reset --stdin'
+
+. ./test-lib.sh
+
+test_expect_success 'reset --stdin' '
+ test_commit hello &&
+ git rm hello.t &&
+ test -z "$(git ls-files hello.t)" &&
+ echo hello.t | git reset --stdin &&
+ test hello.t = "$(git ls-files hello.t)"
+'
+
+test_expect_success 'reset --stdin -z' '
+ test_commit world &&
+ git rm hello.t world.t &&
+ test -z "$(git ls-files hello.t world.t)" &&
+ printf world.tQworld.tQhello.tQ | q_to_nul | git reset --stdin -z &&
+ printf "hello.t\nworld.t\n" >expect &&
+ git ls-files >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--stdin requires --mixed' '
+ echo hello.t >list &&
+ test_must_fail git reset --soft --stdin <list &&
+ test_must_fail git reset --hard --stdin <list &&
+ git reset --mixed --stdin <list
+'
+
+test_done
+
--
2.10.1.513.g00ef6dd
^ permalink raw reply related
* [PATCH 0/2] Support `git reset --stdin`
From: Johannes Schindelin @ 2016-10-11 16:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
This feature was missing, and made it cumbersome for third-party
tools to reset a lot of paths in one go.
Support for --stdin has been added, following builtin/checkout-index.c's
example.
Johannes Schindelin (2):
reset: fix usage
reset: support the --stdin option
Documentation/git-reset.txt | 10 +++++++-
builtin/reset.c | 56 +++++++++++++++++++++++++++++++++++++++++++--
t/t7107-reset-stdin.sh | 33 ++++++++++++++++++++++++++
3 files changed, 96 insertions(+), 3 deletions(-)
create mode 100755 t/t7107-reset-stdin.sh
base-commit: 8a36cd87b7c85a651ab388d403629865ffa3ba0d
Published-As: https://github.com/dscho/git/releases/tag/reset-stdin-v1
Fetch-It-Via: git fetch https://github.com/dscho/git reset-stdin-v1
--
2.10.1.513.g00ef6dd
^ permalink raw reply
* [PATCH 1/2] reset: fix usage
From: Johannes Schindelin @ 2016-10-11 16:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1476202100.git.johannes.schindelin@gmx.de>
The <tree-ish> parameter is actually optional (see man page).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/reset.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 5aa8607..c04ac07 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -24,7 +24,7 @@
static const char * const git_reset_usage[] = {
N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
- N_("git reset [-q] <tree-ish> [--] <paths>..."),
+ N_("git reset [-q] [<tree-ish>] [--] <paths>..."),
N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),
NULL
};
--
2.10.1.513.g00ef6dd
^ permalink raw reply related
* Re: Formatting problem send_mail in version 2.10.0
From: Jeff King @ 2016-10-11 16:02 UTC (permalink / raw)
To: Matthieu Moy
Cc: Larry Finger, Mathieu Lienard--Mayor, Jorge Juan Garcia Garcia,
Remi Lespinet, git
In-Reply-To: <vpqfuo3l4fl.fsf@anie.imag.fr>
On Tue, Oct 11, 2016 at 09:39:58AM +0200, Matthieu Moy 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+]
>
> Is this RFC2822 compliant (https://tools.ietf.org/html/rfc2822)? Not an
> expert of the norm, but my understanding is that you're allowed to use
> either "Name <addr@domain.com>" (name-addr) or addr@domain.com
> (addr-spec), and that comments are allowed within parenthesis like
> "Stable <stable@vger.kernel.org> (4.8+)".
I'm not sure. I don't recall seeing anything like it in the wild before,
but I find it interesting that we behave differently than Mail::Address
(which I generally assume to adhere to a mix of spec and common
practice). I couldn't find anything relevant in rfc2822.
> What is this [4.8+] supposed to mean?
>
> The guilty function is parse_mailboxes in perl/Git.pm. It should be
> rather easy to modify it but I need to understand the spec before I can
> try to implement anything.
It seems to be syntactically invalid as an rfc2822 address. If it's in
common use on trailer lines[1], I think the only sane things are to drop
it, or to stick it in the name. Between the two, I'd argue for the
latter, as that matches what Git historically has done.
I also found it interesting that:
Cc: Stable [4.8+] <stable@vger.kernel.org>
ends up as:
Cc: "Stable [ v4 . 8+ ]" <stable@vger.kernel.org>
I think this is also syntactically invalid as an rfc2822 address, but we
have a habit in Git of treating single-line "name <email>" in a pretty
lax manner, and just letting any character except "<" exist in the name
field. I wonder if we should do something similar here.
-Peff
[1] Running `git log | grep -i '^ *cc:' | grep '\['` on linux.git
shows that it is a common pattern there, though there are other uses
of brackets that probably would not want to include their contents
in the name.
It also looks like:
Cc: stable@vger.kernel.org # 4.8+
is a common pattern.
So I suspect we really are in the realm of micro-formats here, and
it is less about what rfc2822 says, and what people would find it
useful for send-email to do with these bits after the address (and
possibly what other people's scripts do with them).
^ permalink raw reply
* Re: git merge deletes my changes
From: Jakub Narębski @ 2016-10-11 15:56 UTC (permalink / raw)
To: Paul Smith, Eduard Egorov, 'git@vger.kernel.org'
In-Reply-To: <1476121942.15746.99.camel@mad-scientist.net>
W dniu 10.10.2016 o 19:52, Paul Smith pisze:
> On Mon, 2016-10-10 at 10:19 +0000, Eduard Egorov wrote:
>> # ~/gitbuild/git-2.10.1/git merge -s subtree --squash ceph_ansible
>>
>> Can somebody confirm this please? Doesn't "merge -s subtree" really
>> merges branches?
>
> I think possibly you're not fully understanding what the --squash flag
> does... that's what's causing your issue here, not the "-s" option.
>
> A squash merge takes the commits that would be merged from the origin
> branch and squashes them into a single patch and applies them to the
> current branch as a new commit... but this new commit is not a merge
> commit (that is, when you look at it with "git show" etc. the commit
> will have only one parent, not two--or more--parents like a normal merge
> commit).
>
> Basically, it's syntactic sugar for a diff plus patch operation plus
> some Git goodness wrapped around it to make it easier to use.
Actually this is full merge + commit surgery (as if you did merge with
--no-commit, then deleted MERGE_HEAD); the state of worktree is as if
it were after a merge.
>
> But ultimately once you're done, Git has no idea that this new commit
> has any relationship whatsoever to the origin branch. So the next time
> you merge, Git doesn't know that there was a previous merge and it will
> try to merge everything from scratch rather than starting at the
> previous common merge point.
>
> So either you'll have to use a normal, non-squash merge, or else you'll
> have to tell Git by hand what the previous common merge point was (as
> Jeff King's excellent email suggests). Or else, you'll have to live
> with this behavior.
The `git subtree` command (from contrib) allows yet another way: it
squashes *history* of merged subproject (as if with interactive rebase
'squash'), then merges this squash commit.
Now I know why this feature is here...
--
Jakub Narębski
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Larry Finger @ 2016-10-11 15:42 UTC (permalink / raw)
To: Matthieu Moy, Jeff King
Cc: Mathieu Lienard--Mayor, Jorge Juan Garcia Garcia, Remi Lespinet,
git
In-Reply-To: <vpqfuo3l4fl.fsf@anie.imag.fr>
On 10/11/2016 02:39 AM, Matthieu Moy wrote:
> Jeff King <peff@peff.net> writes:
>
>> [+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+]
>
> Is this RFC2822 compliant (https://tools.ietf.org/html/rfc2822)? Not an
> expert of the norm, but my understanding is that you're allowed to use
> either "Name <addr@domain.com>" (name-addr) or addr@domain.com
> (addr-spec), and that comments are allowed within parenthesis like
> "Stable <stable@vger.kernel.org> (4.8+)".
>
> What is this [4.8+] supposed to mean?
>
> The guilty function is parse_mailboxes in perl/Git.pm. It should be
> rather easy to modify it but I need to understand the spec before I can
> try to implement anything.
That added information at the end is intended to be passed on to the stable
group. In this case, the patch needs to be applied to kernel versions 4.8 and later.
Placing the info inside parentheses causes it to be dropped from the outgoing
mail as follows:
(body) Adding cc: Stable <stable@vger.kernel.org> (4.8+) from line 'Cc: Stable
<stable@vger.kernel.org> (4.8+)'
--snip--
Cc: Stable <stable@vger.kernel.org>,
Larry
^ permalink raw reply
* Re: Bug? git worktree fails with master on bare repo
From: Kevin Daudt @ 2016-10-11 15:41 UTC (permalink / raw)
To: Michael Tutty
Cc: Duy Nguyen, Dennis Kaarsemaker, Git Mailing List,
Michael Rappazzo
In-Reply-To: <CAOi_75KR+AEAD540L=GtQ7jH5ngTFdHMq53+FzjzAfsZtTqfwg@mail.gmail.com>
On Mon, Oct 10, 2016 at 08:06:47AM -0500, Michael Tutty wrote:
>
> > If source repo's HEAD is "master", I got the same behavior (worktree add fails)
>
> So if it's possible for a bare repo to have HEAD pointing at master,
> is there a safe way for me to change this (e.g., as a cleanup step
> before doing my actual merge process)?
You can change where HEAD points to in a bare repositor with `git
symbolic-ref HEAD <branch>`, but note that this changes what branch gets
checked out when cloning a repository.
Where users would normally check out master by default, after changing
what HEAD points to, it would be that branch (or detached when HEAD
doesn't even point at a branch).
^ permalink raw reply
* Re: [PATCH] contrib: add credential helper for libsecret
From: Dennis Kaarsemaker @ 2016-10-11 15:01 UTC (permalink / raw)
To: Jeff King; +Cc: Mantas Mikulėnas, git
In-Reply-To: <20161010204654.krj44nk6xbjh4t2v@sigill.intra.peff.net>
On Mon, 2016-10-10 at 16:46 -0400, Jeff King wrote:
> 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?
Doh.
I had done git config --global credential.helper ~/code/git/contrib/credential/libsecret/
After doing it properly (actual path to the binary), it works just
fine. The error message above is slightly suboptimal, but there's no
solution for pebkac.
D.
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Jeff King @ 2016-10-11 15:01 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Duy Nguyen, Johannes Sixt, Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.DEB.2.20.1610111142490.35196@virtualbox>
On Tue, Oct 11, 2016 at 11:44:50AM +0200, Johannes Schindelin wrote:
> > Yeah, that's reasonable, too. So:
> >
> > [alias]
> > d2u = "!dos2unix"
> >
> > acts exactly as if:
> >
> > [alias "d2u"]
> > command = dos2unix
> > type = shell
> >
> > was specified at that point, which is easy to understand.
>
> It is easy to understand, and even easier to get wrong or out of sync. I
> really liked the ease of *one* `git config` call to add new aliases. Not
> sure that I like the need for more such calls just to add *one* alias (one
> config call for "shell", one for "don't cd up", etc).
Could we simply support alias.d2u indefinitely, and you could use
whichever format you felt like (the shorter, more limited one if you
wanted, or the more verbose but flexible one)?
-Peff
^ permalink raw reply
* Re: [PATCH] contrib: add credential helper for libsecret
From: Dennis Kaarsemaker @ 2016-10-11 15:01 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:
> + s = g_hash_table_lookup(attributes, "user");
> + if (s) {
> + g_free(c->username);
> + c->username = g_strdup(s);
> + }
This always overwrites c->username, the original gnome-keyring version
only does that when the username isn't set. Other than that it looks
good to me.
Reviewed-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Tested-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
D.
^ permalink raw reply
* Re: git filter-branch --subdirectory-filter not working as expected, history of other folders is preserved
From: Seaders Oloinsigh @ 2016-10-11 13:56 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161010181907.bqekupn6npuimbir@sigill.intra.peff.net>
On Mon, Oct 10, 2016 at 7:19 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Oct 10, 2016 at 05:12:25PM +0100, Seaders Oloinsigh wrote:
>
>> Due to the structure of this repo, it looks like there are some
>> branches that never had anything to do with the android/ subdirectory,
>> so they're not getting wiped out. My branch is in a better state to
>> how I want it, but still, if I run your suggestion,
>> [...]
>
> Hmm. Yeah, I think this is an artifact of the way that filter-branch
> works with pathspec limiting. It keeps a mapping of commits that it has
> rewritten (including ones that were rewritten only because their
> ancestors were), and realizes that a branch ref needs updated when the
> commit it points to was rewritten.
>
> But if we don't touch _any_ commits in the history reachable from a
> branch (because they didn't even show up in our pathspec-limited
> rev-list), then it doesn't realize we touched the branch's history at
> all.
>
> I agree that the right outcome is for it to delete those branches
> entirely. I suspect the fix would be pretty tricky, though.
>
> In the meantime, I think you can work around it by either:
>
> 1. Make a pass beforehand for refs that do not touch your desired
> paths at all, like:
>
> path=android ;# or whatever
> git for-each-ref --format='%(refname)' |
> while read ref; do
> if test "$(git rev-list --count "$ref" -- "$path")" = 0; then
> echo "delete $ref"
> fi
> done |
> git update-ref --stdin
>
> and then filter what's left:
>
> git filter-branch --subdirectory-filter $path -- --all
This is the perfect solution for me. Going through the delete
branches runthrough also quickened the filter-branch command, and I'm
left with a much more complete version of where I want to be.
I would still contend that the filter-branch either doesn't work as
expected, or the docs need updating to provide extra steps like you've
done, because when dealing with a large repo like we have, running
multiple filter-branch commands, trying different combinations is
quite a time sync, when you're left with the same incorrect solution
again and again.
>
> or
>
> 2. Do the filter-branch, and because you know you specified --all and
> that your filters would touch all histories, any ref which _wasn't_
> touched can be deleted. That list is anything which didn't get a
> backup entry in refs/original. So something like:
>
> git for-each-ref --format='%(refname)' |
> perl -lne 'print $1 if m{^refs/original/(.*)}' >backups
>
> git for-each-ref --format='%(refname)' |
> grep -v ^refs/original >refs
>
> comm -23 refs backups |
> sed "s/^/delete /" |
> git update-ref --stdin
>
> -Peff
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Jakub Narębski @ 2016-10-11 13:24 UTC (permalink / raw)
To: SZEDER Gábor, Duy Nguyen
Cc: Johannes Schindelin, Junio C Hamano, Git Mailing List,
Johannes Sixt
In-Reply-To: <20161011135107.Horde.XsPGOYSpK0L1qW2xMcGqD1i@webmail.informatik.kit.edu>
W dniu 11.10.2016 o 13:51, SZEDER Gábor pisze:
> Quoting Duy Nguyen <pclouds@gmail.com>:
>> On Fri, Oct 7, 2016 at 10:55 PM, Jakub Narębski <jnareb@gmail.com> wrote:
>>> W dniu 07.10.2016 o 16:19, Johannes Schindelin pisze:
>>>> On Fri, 7 Oct 2016, Jakub Narębski wrote:
>>>
>>>>> Note that we would have to teach git completion about new syntax;
>>>>> or new configuration variable if we go that route.
>>>>
>>>> Why would we? Git's completion does not expand aliases, it only completes
>>>> the aliases' names, not their values.
>>>
>>> Because Git completion finds out which _options_ and _arguments_
>>> to complete for an alias based on its expansion.
>>>
>>> Yes, this is nice bit of trickery...
>>
>> It's c63437c (bash: improve aliased command recognition - 2010-02-23)
>> isn't it? This may favor j6t's approach [1] because retrieving alias
>> attributes is much easier.
>>
>> [1] https://public-inbox.org/git/20161006190720.4ecf3ptl6msztoya@sigill.intra.peff.net/T/#mb1d7b8f31d595b05105b8ea2137756761e13dbf4
>> --
>> Duy
>
> The completion script is concerned in three ways:
>
> 1. it has to get the names of all aliases, to offer them along with
> git commands for 'git <TAB>' or 'git help <TAB>',
>
> 2. it has to get the command executed in place of the alias, and
>
> 3. strip everything that can't be a git command, so it can offer the
> right options for the aliased command.
There is also a possibility to tell the completion script which git
command to use for option completion via some trickery, even if the
first git command of many used in script is not right (e.g. when
"$@" is passed somewhere in the middle).
I don't remember exact details, but let's look at source:
# If you use complex aliases of form '!f() { ... }; f', you can use the null
# command ':' as the first command in the function body to declare the desired
# completion style. For example '!f() { : git commit ; ... }; f' will
# tell the completion to use commit completion. This also works with aliases
# of form "!sh -c '...'". For example, "!sh -c ': git commit ; ... '".
Very nice.
>
> The '!!' syntax is the easiest, I think it will just work even with
> the current completion code, no changes necessary.
>
> The '(nocd)' form is still easy, we only have to add this '(nocd)' to
> that list of stripped words for (3), but no further changes necessary
> for (1) and (2).
Shouldn't the '!' vs '!!' / '(nocd)!' affect pathname completion?
Or do tab completion in subdir offer wrong completion of filenames
for aliases?
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v10 04/14] run-command: add clean_on_exit_handler
From: Johannes Schindelin @ 2016-10-11 12:12 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, gitster, jnareb, peff
In-Reply-To: <20161008112530.15506-5-larsxschneider@gmail.com>
Hi Lars,
On Sat, 8 Oct 2016, larsxschneider@gmail.com wrote:
> @@ -31,6 +32,15 @@ static void cleanup_children(int sig, int in_signal)
> while (children_to_clean) {
> struct child_to_clean *p = children_to_clean;
> children_to_clean = p->next;
> +
> + if (p->process && !in_signal) {
> + struct child_process *process = p->process;
> + if (process->clean_on_exit_handler) {
> + trace_printf("trace: run_command: running exit handler for pid %d", p->pid);
On Windows, pid_t translates to long long int, resulting in this build
error:
-- snip --
In file included from cache.h:10:0,
from run-command.c:1:
run-command.c: In function 'cleanup_children':
run-command.c:39:18: error: format '%d' expects argument of type 'int', but argument 5 has type 'pid_t {aka long long int}' [-Werror=format=]
trace_printf("trace: run_command: running exit handler for pid %d", p->pid);
^
trace.h:81:53: note: in definition of macro 'trace_printf'
trace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__)
^~~~~~~~~~~
cc1.exe: all warnings being treated as errors
make: *** [Makefile:1987: run-command.o] Error 1
-- snap --
Maybe use PRIuMAX as we do elsewhere (see output of `git grep
printf.*pid`):
trace_printf("trace: run_command: running exit handler for pid %"
PRIuMAX, (uintmax_t)p->pid);
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: SZEDER Gábor @ 2016-10-11 11:51 UTC (permalink / raw)
To: Duy Nguyen
Cc: Jakub Narębski, Johannes Schindelin, Junio C Hamano,
Git Mailing List, Johannes Sixt
In-Reply-To: <CACsJy8Bip0mscutDHib-2O1g+eN2twO0m+OyOg2BTUudjOdwfw@mail.gmail.com>
Quoting Duy Nguyen <pclouds@gmail.com>:
> On Fri, Oct 7, 2016 at 10:55 PM, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 07.10.2016 o 16:19, Johannes Schindelin pisze:
>>> On Fri, 7 Oct 2016, Jakub Narębski wrote:
>>
>>>> Note that we would have to teach git completion about new syntax;
>>>> or new configuration variable if we go that route.
>>>
>>> Why would we? Git's completion does not expand aliases, it only completes
>>> the aliases' names, not their values.
>>
>> Because Git completion finds out which _options_ and _arguments_
>> to complete for an alias based on its expansion.
>>
>> Yes, this is nice bit of trickery...
>
> It's c63437c (bash: improve aliased command recognition - 2010-02-23)
> isn't it? This may favor j6t's approach [1] because retrieving alias
> attributes is much easier.
>
> [1]
> https://public-inbox.org/git/20161006190720.4ecf3ptl6msztoya@sigill.intra.peff.net/T/#mb1d7b8f31d595b05105b8ea2137756761e13dbf4
> --
> Duy
The completion script is concerned in three ways:
1. it has to get the names of all aliases, to offer them along with
git commands for 'git <TAB>' or 'git help <TAB>',
2. it has to get the command executed in place of the alias, and
3. strip everything that can't be a git command, so it can offer the
right options for the aliased command.
The '!!' syntax is the easiest, I think it will just work even with
the current completion code, no changes necessary.
The '(nocd)' form is still easy, we only have to add this '(nocd)' to
that list of stripped words for (3), but no further changes necessary
for (1) and (2).
'alias.d2u.command' is tricky. Both (1) and (2) require adjustments,
preferably in a way that doesn't involve additional git processes, at
least for (1), as it is executed often, for every 'git <TAB>', for the
sake of users on platforms with not particularly stellar fork()+exec()
performance. I think it would be possible to have only one 'git
config --get-regexp' and a little bit of post processing in each case,
but I didn't think too hard about possible pitfalls, especially when
dealing with ambiguity when both 'alias.d2u' and 'alias.d2u.command'
are set. And I won't think any more about it until a conclusion is
reached that we'll go this route :)
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Johannes Schindelin @ 2016-10-11 11:28 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Jeff King, Johannes Sixt, Junio C Hamano, Git Mailing List
In-Reply-To: <CACsJy8Aufm7g9SnckiXbMCOOvsLMqESRHLR+Zd94HxPeJj=gTw@mail.gmail.com>
Hi Duy,
On Tue, 11 Oct 2016, Duy Nguyen wrote:
> On Tue, Oct 11, 2016 at 4:44 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Sun, 9 Oct 2016, Jeff King wrote:
> >
> >> On Sun, Oct 09, 2016 at 06:32:38PM +0700, Duy Nguyen wrote:
> >>
> >> > > If you mean ambiguity between the old "alias.X" and the new "alias.X.*",
> >> > > then yes, I think that's an unavoidable part of the transition. IMHO,
> >> > > the new should take precedence over the old, and people will gradually
> >> > > move from one to the other.
> >> >
> >> > Do we really need to treat this differently than
> >> >
> >> > [alias]
> >> > d2u = !dos2unix
> >> > d2u = C:/cygwin/bin/dos3unix.exe
> >> >
> >> > ?
> >> >
> >> > Another similar case is one d2u (could be either old syntax or new) is
> >> > defined in ~/.gitconfig and the other d2u in $GIT_DIR/config. In
> >> > either case, the "latest" d2u definition wins.
> >>
> >> Yeah, that's reasonable, too. So:
> >>
> >> [alias]
> >> d2u = "!dos2unix"
> >>
> >> acts exactly as if:
> >>
> >> [alias "d2u"]
> >> command = dos2unix
> >> type = shell
> >>
> >> was specified at that point, which is easy to understand.
> >
> > It is easy to understand, and even easier to get wrong or out of sync. I
> > really liked the ease of *one* `git config` call to add new aliases.
>
> I was about to bring this up. Although to me, "git config --global
> alias.foo bar" is more convenient, but not using it is not exactly
> easy to get wrong or out of sync. For adding alias.$name.* I was
> thinking about "git config --global --edit", not executing "git
> config" multiple times.
Right, but many of my aliases get set by scripts, so your `--edit` idea
won't work for me.
> > Not sure that I like the need for more such calls just to add *one* alias (one
> > config call for "shell", one for "don't cd up", etc).
>
> We could add git-alias if more alias types pop up (and in my opinion
> git-alias is the right call, we've been abusing git-config for alias
> manipulation for a long time).
Maybe.
It is also possible that this issue is a good indicator that we are
complicating things [*1*] more than necessary...
Ciao,
Dscho
Footnote *1*:
http://thedailywtf.com/articles/The_Complicator_0x27_s_Gloves
^ permalink raw reply
* Re: Feature request: use relative path in worktree config files
From: Duy Nguyen @ 2016-10-11 11:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stéphane Klein, Git Mailing List
In-Reply-To: <xmqq60p0f3un.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 11, 2016 at 1:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>>> I think there are some pros and some cons for relative path and absolute path.
>>> Maybe append a "--relative" option with `git worktree add` ?
>>>
>>> I've converted all path to relative and all work with success.
>>>
>>> What do you think to append this --relative option.
>>
>> Patches are welcome.
>
> Hmm, are they really welcome?
>
> Is an invocation of "git worktree add" really the right point in the
> workflow to decide if these references to other repositories should
> be relative or absolute? When you are moving referrer, it is more
> convenient if the reference uses absolute path to name the
> referrent. When you are moving both referrer and referrent, it is
> more convenient to use relative.
You're right (thanks for thinking this through). There's another case
that benefits from relative paths besides Stephane's use case: if you
put the entire repo and all associated worktrees on a portable disk,
the disk could be mounted on different paths each time, so absolute
paths do not fly.
> I somehow doubt that users know which future move they would be
> making when doing "git worktree add".
>
> To me, this almost looks like a need for a new subcommand to "git
> worktree" that lets you move existing worktree to elsewhere, or turn
> absolute reference to relative and vice versa.
An alternative is always use relative paths. You have to move
worktrees with "git worktree move". Which simplifies things a bit
(absolute -> relative conversion on existing worktrees could be done
at move time). But I'm not sure if it's a good idea to kill manual
move support. If you accidentally move a worktree, it'll be
disconnected and you have to fix that by yourself.
--
Duy
^ permalink raw reply
* Re: [PATCH] pretty: respect color settings for %C placeholders
From: Duy Nguyen @ 2016-10-11 10:59 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Tom Hale, git, Junio C Hamano
In-Reply-To: <20161010151517.6wszhuyp57yfncaj@sigill.intra.peff.net>
On Mon, Oct 10, 2016 at 10:15 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Oct 10, 2016 at 10:28:18AM -0400, Jeff King wrote:
>
>> > We could add some new tag to change the behavior of all following %C
>> > tags. Something like %C(tty) maybe (probably a bad name), then
>> > discourage the use if "%C(auto" for terminal detection?
>>
>> Yeah, adding a "%C(enable-auto-color)" or something would be backwards
>> compatible and less painful than using "%C(auto)" everywhere. I do
>> wonder if anybody actually _wants_ the "always show color, even if
>> --no-color" behavior. I'm having trouble thinking of a good use for it.
>>
>> IOW, I'm wondering if anyone would disagree that the current behavior is
>> simply buggy. Reading the thread at:
>>
>> http://public-inbox.org/git/7v4njkmq07.fsf@alter.siamese.dyndns.org/
>>
>> I don't really see any compelling reason against it (there was some
>> question of which config to use, but we already answered that with
>> "%C(auto)", and use the value from the pretty_ctx).
>
> So here's what a patch to do that would look like. I admit that "I can't
> think of a good use" does not mean there _isn't_ one, but perhaps by
> posting this, it might provoke other people to think on it, too. And if
> nobody can come up with, maybe it's a good idea.
I think you covered all bases with %C(always,..) and updating
for-each-ref code. And changing behavior of visual features like this
sounds more like "evolving" than "breaking backward compatibility" to
me. So +1.
--
Duy
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Johannes Schindelin @ 2016-10-11 10:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610111221530.35196@virtualbox>
Hi Junio,
On Tue, 11 Oct 2016, Johannes Schindelin wrote:
> On Tue, 11 Oct 2016, Johannes Schindelin wrote:
>
> > -- snipsnap --
> > @@ -906,11 +904,13 @@ static int walk_revs_populate_todo(struct todo_list
> > *todo_list,
> > item->command = command;
> > item->commit = commit;
> > item->offset_in_buf = todo_list->buf.len;
> > + strbuf_addstr(&todo_list->buf, command);
>
> This would be command_string instead of command, of course.
>
> > + strbuf_addch(&todo_list->buf, ' ');
> > + strbuf_add_unique_abbrev(&todo_list->buf,
> > + commit->object.oid.hash,
> > + DEFAULT_ABBREV);
> > 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);
> > + strbuf_add(&todo_list->buf, subject, subject_len);
> > unuse_commit_buffer(commit, commit_buffer);
> > }
> > return 0;
In the end, I decided to actually *not* use strbuf_add_unique_abbrev()
here because it really makes the code very much too ugly after the
introduction of short_commit_name():
-- snip --
@@ -1093,10 +1093,16 @@ static int walk_revs_populate_todo(struct
todo_list *todo_list,
item->arg = NULL;
item->arg_len = 0;
item->offset_in_buf = todo_list->buf.len;
+ strbuf_addstr(&todo_list->buf, command_string);
+ strbuf_addch(&todo_list->buf, ' ');
+ strbuf_add_unique_abbrev(&todo_list->buf,
+ commit->object.oid.hash,
+ DEFAULT_ABBREV);
+ strbuf_addch(&todo_list->buf, ' ');
subject_len = find_commit_subject(commit_buffer, &subject);
- strbuf_addf(&todo_list->buf, "%s %s %.*s\n",
command_string,
- short_commit_name(commit), subject_len, subject);
+ strbuf_add(&todo_list->buf, subject, subject_len);
unuse_commit_buffer(commit, commit_buffer);
+ strbuf_addch(&todo_list->buf, '\n');
}
return 0;
}
-- snap --
I hope you will forgive me disagreeing with you here.
To make it easier to accept, I reordered the short_commit_name() patch so
it comes before revamping the todo parsing.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Duy Nguyen @ 2016-10-11 10:53 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, Johannes Sixt, Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.DEB.2.20.1610111142490.35196@virtualbox>
On Tue, Oct 11, 2016 at 4:44 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 9 Oct 2016, Jeff King wrote:
>
>> On Sun, Oct 09, 2016 at 06:32:38PM +0700, Duy Nguyen wrote:
>>
>> > > If you mean ambiguity between the old "alias.X" and the new "alias.X.*",
>> > > then yes, I think that's an unavoidable part of the transition. IMHO,
>> > > the new should take precedence over the old, and people will gradually
>> > > move from one to the other.
>> >
>> > Do we really need to treat this differently than
>> >
>> > [alias]
>> > d2u = !dos2unix
>> > d2u = C:/cygwin/bin/dos3unix.exe
>> >
>> > ?
>> >
>> > Another similar case is one d2u (could be either old syntax or new) is
>> > defined in ~/.gitconfig and the other d2u in $GIT_DIR/config. In
>> > either case, the "latest" d2u definition wins.
>>
>> Yeah, that's reasonable, too. So:
>>
>> [alias]
>> d2u = "!dos2unix"
>>
>> acts exactly as if:
>>
>> [alias "d2u"]
>> command = dos2unix
>> type = shell
>>
>> was specified at that point, which is easy to understand.
>
> It is easy to understand, and even easier to get wrong or out of sync. I
> really liked the ease of *one* `git config` call to add new aliases.
I was about to bring this up. Although to me, "git config --global
alias.foo bar" is more convenient, but not using it is not exactly
easy to get wrong or out of sync. For adding alias.$name.* I was
thinking about "git config --global --edit", not executing "git
config" multiple times.
> Not sure that I like the need for more such calls just to add *one* alias (one
> config call for "shell", one for "don't cd up", etc).
We could add git-alias if more alias types pop up (and in my opinion
git-alias is the right call, we've been abusing git-config for alias
manipulation for a long time).
--
Duy
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Johannes Schindelin @ 2016-10-11 10:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610111212090.35196@virtualbox>
Hi Junio,
On Tue, 11 Oct 2016, Johannes Schindelin wrote:
> -- snipsnap --
> @@ -906,11 +904,13 @@ static int walk_revs_populate_todo(struct todo_list
> *todo_list,
> item->command = command;
> item->commit = commit;
> item->offset_in_buf = todo_list->buf.len;
> + strbuf_addstr(&todo_list->buf, command);
This would be command_string instead of command, of course.
> + strbuf_addch(&todo_list->buf, ' ');
> + strbuf_add_unique_abbrev(&todo_list->buf,
> + commit->object.oid.hash,
> + DEFAULT_ABBREV);
> 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);
> + strbuf_add(&todo_list->buf, subject, subject_len);
> unuse_commit_buffer(commit, commit_buffer);
> }
> return 0;
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Johannes Schindelin @ 2016-10-11 10:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqqlgxvdf90.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 10 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > 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.
Urgh. Thanks for pointing this out to me, though. Will be fixed in the
next iteration.
> > + 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.
Right, I actually looked at this place when I tried to decide where I
could use that function. Somehow I thought I'd not break up the flow here.
But since you asked so nicely, I'll squash this in (I personally find it
uglier, and longer, but it does use strbuf_add_unique_abbrev() now):
-- snipsnap --
@@ -906,11 +904,13 @@ static int walk_revs_populate_todo(struct todo_list
*todo_list,
item->command = command;
item->commit = commit;
item->offset_in_buf = todo_list->buf.len;
+ strbuf_addstr(&todo_list->buf, command);
+ strbuf_addch(&todo_list->buf, ' ');
+ strbuf_add_unique_abbrev(&todo_list->buf,
+ commit->object.oid.hash,
+ DEFAULT_ABBREV);
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);
+ strbuf_add(&todo_list->buf, subject, subject_len);
unuse_commit_buffer(commit, commit_buffer);
}
return 0;
^ 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