* Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call
From: Heiko Voigt @ 2016-10-12 13:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Stefan Beller, git, Jens.Lehmann, Fredrik Gustafsson,
Leandro Lucarella
In-Reply-To: <xmqqlgxvbype.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 10, 2016 at 03:56:13PM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
>
> > -static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])
> > +static int check_has_hash(const unsigned char sha1[20], void *data)
> > {
> > - if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
> > + int *has_hash = (int *) data;
> > +
> > + if (!lookup_commit_reference(sha1))
> > + *has_hash = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int submodule_has_hashes(const char *path, struct sha1_array *hashes)
> > +{
> > + int has_hash = 1;
> > +
> > + if (add_submodule_odb(path))
> > + return 0;
> > +
> > + sha1_array_for_each_unique(hashes, check_has_hash, &has_hash);
> > + return has_hash;
> > +}
> > +
> > +static int submodule_needs_pushing(const char *path, struct sha1_array *hashes)
> > +{
> > + if (!submodule_has_hashes(path, hashes))
> > return 0;
>
> Same comment about naming.
>
> What do check-has-hash and submodule-has-hashes exactly mean by
> "hash" in their names? Because I think what is checked here is
> "does the local submodule repository have _all_ the commits
> referenced from the superproject commit we are pushing?", so I'd
> prefer to see "commit" in their names.
>
> If we do not even have these commits locally, then there is no point
> attempting to push, so returning 0 (i.e. it is not "needs pushing"
> situation) is correct but it is a but subtle. It's not "we know
> they already have them", but it is "even if we tried to push, it
> won't do us or the other side any good." A single-liner in-code
> comment may help.
First the naming part. How about:
submodule_has_commits()
?
Second as mentioned a previous answer[1] to this part: I would actually
like to have a die() here instead of blindly proceeding. Since the user
either specified --recurse-submodules=... at the commandline or it was
implicitly enabled because we have submodules in the tree we should be
careful and not push revisions referencing submodules that are not
available at a remote. If we can not properly figure it out I would
suggest to stop and tell the user how to solve the situation. E.g.
either she clones the appropriate submodules or specifies
--no-recurse-submodules on the commandline to tell git that she does not
care.
Returning 0 here means: "No push needed" but the correct answer would
be: "We do not know". Question is what we should do here which I am
planning to address in a separate patch series since that will be
changing behavior.
So how about:
if (!submodule_has_hashes(path, hashes))
/* NEEDSWORK: The correct answer here is "We do not
* know" instead of "No". We currently proceed pushing
* here as if the submodules commits are available on a
* remote, which is not always correct. */
return 0;
What do you think?
Cheers Heiko
[1] http://public-inbox.org/git/20160919195812.GC62429@book.hvoigt.net/
^ permalink raw reply
* Re: interactive rebase should better highlight the not-applying commit
From: Joshua N Pritikin @ 2016-10-12 13:27 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kZSQx7aOCgQ2dwzJeCLX-k-+x1SKabEBG7CktNfeXAbvg@mail.gmail.com>
On Tue, Oct 11, 2016 at 01:55:22PM -0700, Stefan Beller wrote:
> On Tue, Oct 11, 2016 at 12:07 PM, Joshua N Pritikin <jpritikin@pobox.com> wrote:
> > I assume somebody familiar with GIT's code base could make this change
> > in about 10 minutes.
>
> Can you elaborate how you come to that estimate?
Hm, a false belief in the general awesomeness of GIT developers?
On Tue, Oct 11, 2016 at 02:25:19PM -0700, Stefan Beller wrote:
> On Tue, Oct 11, 2016 at 12:07 PM, Joshua N Pritikin <jpritikin@pobox.com> wrote:
> > As of GIT 2.8.1, if you do an interactive rebase and get some conflict
> > in the stack of patches then the commit with the conflict is buried in
> > 4-5 lines of output. It is visually difficult to immediately pick out
> > which commit did not apply cleanly. I suggest highlighting the 1 line
> > commit summary in red or green or some color to help it stand out from
> > all the other output.
> >
> > I decided to suggest this change after I realized that I probably
> > skipped a commit during an interactive rebase instead of resolving the
> > conflict. I knew I had to skip some commit so I assumed that I just need
> > to skip without reading the commit summary carefully. Now it is 7-15
> > days after I did the erroneous rebase. I had to spend a few hours today
> > with GIT's archaeology tools to find the lost code.
>
> Looking at the actual code, this is not as easy as one might assume,
> because rebase is written in shell. (One of the last remaining large
> commands in shell), and there is no color support in the die(..)
> function.
I'm sorry to hear that.
> However IIUC currently rebase is completely rewritten/ported to C
> where it is easier to add color support as we do have some color
> support in there already.
Sounds great. Is there a beta release that I can try out?
Also, I have another wishlist item for (interactive) rebase. Sometimes I
do a rebase to fix some tiny thing 10-15 commits from HEAD. Maybe only 1
file is affected and there are no merge conflicts, but when rebase
reapplies all the commits, the timestamps of lots of unmodified files
change even though they are unmodified compared to before the rebase.
Since the modification times are used by 'make' to compute dependencies,
this creates a lot of useless recompilation that slows things down. It
would be great if rebase only changed the timestamps of files that were
actually modified.
Thank you.
--
Joshua N. Pritikin, Ph.D.
Virginia Institute for Psychiatric and Behavioral Genetics
Virginia Commonwealth University
PO Box 980126
800 E Leigh St, Biotech One, Suite 1-133
Richmond, VA 23219
http://people.virginia.edu/~jnp3bc
^ permalink raw reply
* Re: [PATCH v2 1/3] serialize collection of changed submodules
From: Heiko Voigt @ 2016-10-12 13:11 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Jeff King, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <CAGZ79kZiY56-84aThH1F02E_HzCTAK3KSYLbyP1D5GUAt892cw@mail.gmail.com>
On Fri, Oct 07, 2016 at 10:59:29AM -0700, Stefan Beller wrote:
> On Fri, Oct 7, 2016 at 8:06 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > +static void free_submodules_sha1s(struct string_list *submodules)
> > +{
> > + int i;
> > + for (i = 0; i < submodules->nr; i++) {
> > + struct string_list_item *item = &submodules->items[i];
>
> You do not seem to make use of `i` explicitely, so
> for_each_string_list_item might be more readable here?
Will change.
> > @@ -603,12 +645,23 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
> > die("revision walk setup failed");
> >
> > while ((commit = get_revision(&rev)) != NULL)
> > - find_unpushed_submodule_commits(commit, needs_pushing);
> > + find_unpushed_submodule_commits(commit, &submodules);
> >
> > reset_revision_walk();
> > free(sha1_copy);
> > strbuf_release(&remotes_arg);
> >
> > + for (i = 0; i < submodules.nr; i++) {
> > + struct string_list_item *item = &submodules.items[i];
>
> You do not seem to make use of `i` explicitely, so
> for_each_string_list_item might be more readable here?
As above.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH v2 2/3] serialize collection of refs that contain submodule changes
From: Heiko Voigt @ 2016-10-12 13:10 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Jeff King, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <CAGZ79kZ5CPTuW2fARDs3BUt89b8H_=P3otZv+Vm5nTV70NLWtg@mail.gmail.com>
On Fri, Oct 07, 2016 at 11:16:31AM -0700, Stefan Beller wrote:
> > diff --git a/submodule.c b/submodule.c
> > index 59c9d15905..5044afc2f8 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -522,6 +522,13 @@ static int has_remote(const char *refname, const struct object_id *oid,
> > return 1;
> > }
> >
> > +static int append_hash_to_argv(const unsigned char sha1[20], void *data)
> > +{
> > + struct argv_array *argv = (struct argv_array *) data;
> > + argv_array_push(argv, sha1_to_hex(sha1));
>
> Nit of the day:
> When using the struct child-process, we have the oldstyle argv NULL
> terminated array as
> well as the new style args argv_array. So in that context we'd prefer
> `args` as a name for
> argv_array as that helps to distinguish from the old array type.
> Here however `argv` seems to be a reasonable name, in fact whenever we
> do not deal with
> child processes, we seem to not like the `args` name:
>
> $ git grep argv_array |wc -l
> 577
> $ git grep argv_array |grep args |wc -l
> 293
>
> The rest looks good to me. :)
Thanks. So I do not completely get what you are suggesting: args or kept
it the way it is? Since in the end you are saying it is ok here ;) I
mainly chose this name because I am substituting the argv variable which
is already called 'argv' with this array. That might also be the reason
why in so many locations with struct child_processe's we have the 'argv'
name: Because they initially started with the old-style NULL terminated
array.
I am fine with it either way. Just tell me what you like :)
Cheers Heiko
^ permalink raw reply
* [PATCH v3] gpg-interface: use more status letters
From: Michael J Gruber @ 2016-10-12 13:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Alex
In-Reply-To: <xmqqtwckf5mg.fsf@gitster.mtv.corp.google.com>
According to gpg2's doc/DETAILS:
For each signature only one of the codes GOODSIG, BADSIG,
EXPSIG, EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted.
gpg1 ("classic") behaves the same (although doc/DETAILS differs).
Currently, we parse gpg's status output for GOODSIG, BADSIG and
trust information and translate that into status codes G, B, U, N
for the %G? format specifier.
git-verify-* returns success in the GOODSIG case only. This is
somewhat in disagreement with gpg, which considers the first 5 of
the 6 above as VALIDSIG, but we err on the very safe side.
Introduce additional status codes E, X, Y, R for ERRSIG, EXPSIG,
EXPKEYSIG, and REVKEYSIG so that a user of %G? gets more information
about the absence of a 'G' on first glance.
Requested-by: Alex <agrambot@gmail.com>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
v3 incorporates Junios' changes to the commit message, as well as his
suggestion how to use an undefined gpghome the way test-lib does.
Also, all Y-related changes (including the if in pretty.c).
Testing X, Y, and R from our test scripts is somewhat problematic
(some gpg versions do not allow back-dating, and we cannot ship pre-made
signatures easily) but I have tested all of them locally.
Documentation/pretty-formats.txt | 10 ++++++++--
gpg-interface.c | 13 ++++++++++---
pretty.c | 4 ++++
t/t7510-signed-commit.sh | 13 ++++++++++++-
4 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a942d57f73..179c9389aa 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -143,8 +143,14 @@ ifndef::git-rev-list[]
- '%N': commit notes
endif::git-rev-list[]
- '%GG': raw verification message from GPG for a signed commit
-- '%G?': show "G" for a good (valid) signature, "B" for a bad signature,
- "U" for a good signature with unknown validity and "N" for no signature
+- '%G?': show "G" for a good (valid) signature,
+ "B" for a bad signature,
+ "U" for a good signature with unknown validity,
+ "X" for a good signature that has expired,
+ "Y" for a good signature made by an expired key,
+ "R" for a good signature made by a revoked key,
+ "E" if the signature cannot be checked (e.g. missing key)
+ and "N" for no signature
- '%GS': show the name of the signer for a signed commit
- '%GK': show the key used to sign a signed commit
- '%gD': reflog selector, e.g., `refs/stash@{1}` or
diff --git a/gpg-interface.c b/gpg-interface.c
index 8672edaf48..e44cc27da1 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -33,6 +33,10 @@ static struct {
{ 'B', "\n[GNUPG:] BADSIG " },
{ 'U', "\n[GNUPG:] TRUST_NEVER" },
{ 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
+ { 'E', "\n[GNUPG:] ERRSIG "},
+ { 'X', "\n[GNUPG:] EXPSIG "},
+ { 'Y', "\n[GNUPG:] EXPKEYSIG "},
+ { 'R', "\n[GNUPG:] REVKEYSIG "},
};
void parse_gpg_output(struct signature_check *sigc)
@@ -54,9 +58,12 @@ void parse_gpg_output(struct signature_check *sigc)
/* The trust messages are not followed by key/signer information */
if (sigc->result != 'U') {
sigc->key = xmemdupz(found, 16);
- found += 17;
- next = strchrnul(found, '\n');
- sigc->signer = xmemdupz(found, next - found);
+ /* The ERRSIG message is not followed by signer information */
+ if (sigc-> result != 'E') {
+ found += 17;
+ next = strchrnul(found, '\n');
+ sigc->signer = xmemdupz(found, next - found);
+ }
}
}
}
diff --git a/pretty.c b/pretty.c
index 25efbcac92..d89ca30911 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1232,8 +1232,12 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
switch (c->signature_check.result) {
case 'G':
case 'B':
+ case 'E':
case 'U':
case 'N':
+ case 'X':
+ case 'Y':
+ case 'R':
strbuf_addch(sb, c->signature_check.result);
}
break;
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 6e839f5489..762135adea 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -2,6 +2,7 @@
test_description='signed commit tests'
. ./test-lib.sh
+GNUPGHOME_NOT_USED=$GNUPGHOME
. "$TEST_DIRECTORY/lib-gpg.sh"
test_expect_success GPG 'create signed commits' '
@@ -190,7 +191,7 @@ test_expect_success GPG 'show bad signature with custom format' '
test_cmp expect actual
'
-test_expect_success GPG 'show unknown signature with custom format' '
+test_expect_success GPG 'show untrusted signature with custom format' '
cat >expect <<-\EOF &&
U
61092E85B7227189
@@ -200,6 +201,16 @@ test_expect_success GPG 'show unknown signature with custom format' '
test_cmp expect actual
'
+test_expect_success GPG 'show unknown signature with custom format' '
+ cat >expect <<-\EOF &&
+ E
+ 61092E85B7227189
+
+ EOF
+ GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GK%n%GS" eighth-signed-alt >actual &&
+ test_cmp expect actual
+'
+
test_expect_success GPG 'show lack of signature with custom format' '
cat >expect <<-\EOF &&
N
--
2.10.1.532.gfe29b57
^ permalink raw reply related
* Re: [PATCH v2 1/3] serialize collection of changed submodules
From: Heiko Voigt @ 2016-10-12 13:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Jeff King, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <xmqqvawzbzb2.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 10, 2016 at 03:43:13PM -0700, Junio C Hamano wrote:
> 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.
I did not name this function too precisely to keep it's name short since
everything specific was quite long, like the suggestion from Junio.
Since this is a static function local to the submodule file I was
assuming anyone interested would just look up the usage and immediately
see the purpose. If I look into submodule-cache.c where I have a similar
functionality we used 'lookup_or_create' for this create on demand
functionality. So a function name would be:
lookup_or_create_commit_objects_for_submodule_path(...
Which seems quite extensively long for a static function so how about
we shorten it a bit and add a comment:
/* lookup or create commit object list for submodule */
get_commit_objects_for_submodule_path(...
?
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 2/2] reset: support the --stdin option
From: Johannes Schindelin @ 2016-10-12 12:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqeg3mai1b.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > + 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;
>
> Didn't we already die above under that mode?
Oh right. Copy/paste fail.
> > + 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;
>
> It makes sense to collect, but...
It does, doesn't it? I really would have loved to start resetting right
away, but if the list were not sorted and traversed at the same time as
the tree-ish, the performance would just be suboptimal.
I think that is an important point and I adjusted the commit message
accordingly.
> > + parse_pathspec(&pathspec, 0, flags, prefix,
> > + (const char **)stdin_paths);
>
> ...letting them be used as if they are pathspec is wrong when
> stdin_paths[] contain wildcard, isn't it?
>
> I think flags |= PATHSPEC_LITERAL_PATH can help fixing it. 0/2 said
> this mimicks checkout-index and I think it should by not treating
> the input as wildcarded patterns (i.e. "echo '*.c' | reset --stdin"
> shouldn't be the way to reset all .c files --- that's something we
> would want to add to the test, I guess).
True. I adjust the flags accordingly now.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH 2/2] reset: support the --stdin option
From: Johannes Schindelin @ 2016-10-12 12:39 UTC (permalink / raw)
To: Jakub Narębski; +Cc: git, Junio C Hamano
In-Reply-To: <dc76476b-9ad5-a3b6-f12f-33cda2ca5814@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 455 bytes --]
Hi Kuba,
On Tue, 11 Oct 2016, Jakub Narębski wrote:
> W dniu 11.10.2016 o 18:09, Johannes Schindelin pisze:
>
> > SYNOPSIS
> > --------
> > [verse]
> > -'git reset' [-q] [<tree-ish>] [--] <paths>...
> > +'git reset' [-q] [--stdin [-z]] [<tree-ish>] [--] <paths>...
>
> I think you meant here
>
> +'git reset' [-q] [--stdin [-z]] [<tree-ish>]
Good point. I overlooked that the <paths>... are not optional here.
Thanks,
Dscho
^ permalink raw reply
* [PATCH] t1512: become resilient to GETTEXT_POISON build
From: Vasco Almeida @ 2016-10-12 12:25 UTC (permalink / raw)
To: git; +Cc: Vasco Almeida, Jeff King
The concerned message was marked for translation by 0c99171
("get_short_sha1: mark ambiguity error for translation", 2016-09-26).
Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
---
t/t1512-rev-parse-disambiguation.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 7c659eb..711704b 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -42,7 +42,7 @@ test_expect_success 'blob and tree' '
test_expect_success 'warn ambiguity when no candidate matches type hint' '
test_must_fail git rev-parse --verify 000000000^{commit} 2>actual &&
- grep "short SHA1 000000000 is ambiguous" actual
+ test_i18ngrep "short SHA1 000000000 is ambiguous" actual
'
test_expect_success 'disambiguate tree-ish' '
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v3 05/25] sequencer: eventually release memory allocated for the option values
From: Johannes Schindelin @ 2016-10-12 12:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqq4m4ic0gw.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> 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);
Oh, that xstrdup_or_null() function slipped by me. My local patches use it
now.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 13/25] sequencer: prepare for rebase -i's commit functionality
From: Johannes Schindelin @ 2016-10-12 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt, Michael Haggerty
In-Reply-To: <xmqqwphe8zl2.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> > @@ -370,19 +383,79 @@ static int is_index_unchanged(void)
> > }
> >
> > /*
> > + * Read the author-script file into an environment block, ready for use in
> > + * run_command(), that can be free()d afterwards.
> > + */
> > +static char **read_author_script(void)
> > +{
> > + struct strbuf script = STRBUF_INIT;
> > + int i, count = 0;
> > + char *p, *p2, **env;
> > + size_t env_size;
> > +
> > + if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
> > + return NULL;
> > +
> > + for (p = script.buf; *p; p++)
> > + if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> > + strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> > + else if (*p == '\'')
> > + strbuf_splice(&script, p-- - script.buf, 1, "", 0);
> > + else if (*p == '\n') {
> > + *p = '\0';
> > + count++;
> > + }
>
> Hmph, didn't we recently add parse_key_value_squoted() to build
> read_author_script() in builtin/am.c on top of it, so that this
> piece of code can also take advantage of and share the parser?
I already pointed out that the author-script file may *not* be quoted.
sq_dequote() would return NULL and parse_key_value_squoted() would *fail*.
To complicate things further, the sequencer does not even need to access
the values at all. It needs to pass them to run_command() as an
environment block, which means that we would have to reconstruct the lines
after parse_key_value_squoted() painstakingly untangled the key names from
the values.
In short, this is another instance where using a function just because it
exists and is nominally related would make the resulting patch *more*
complicated than it currently is.
> > +/*
>
> Offtopic: this line and the beginning of the new comment block that
> begins with "Read the author-script" above show a suboptimal marking
> of what is added and what is left. I wonder "diff-indent-heuristic"
> topic by Michael can help to make it look better.
Maybe. I'll try to look into that once the more serious questions about
this patch series have been addressed.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 08/25] sequencer: strip CR from the todo script
From: Johannes Schindelin @ 2016-10-12 11:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqq60oyaf7s.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/sequencer.c b/sequencer.c
> > index 678fdf3..cee7e50 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -774,6 +774,9 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
> >
> > next_p = *eol ? eol + 1 /* skip LF */ : eol;
> >
> > + if (p != eol && eol[-1] == '\r')
> > + eol--; /* skip Carriage Return */
>
> micronit: s/skip/strip/ ;-)
Okay,
Dscho
^ permalink raw reply
* Re: [PATCH v3 12/25] sequencer: remember the onelines when parsing the todo file
From: Johannes Schindelin @ 2016-10-12 11:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqq1szmaemr.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/sequencer.c b/sequencer.c
> > index afc494e..7ba5e07 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -708,6 +708,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> > struct todo_item {
> > enum todo_command command;
> > struct commit *commit;
> > + const char *arg;
> > + int arg_len;
> > size_t offset_in_buf;
>
> micronit: you can make it to size_t and lose the cast below, no?
No. The primary users of arg_len call a printf() style function with %.*s,
expecting an int. So your suggestion would lose one cast, but introduce at
least four casts in return.
Ciao,
Dscho
^ permalink raw reply
* Re: git diff
From: Mike Rappazzo @ 2016-10-12 11:06 UTC (permalink / raw)
To: webmaster; +Cc: git
In-Reply-To: <1066408917.43087.1476269456819.JavaMail.open-xchange@app06.ox.hosteurope.de>
On Wed, Oct 12, 2016 at 6:50 AM, <webmaster@peter-speer.de> wrote:
> Hi.
>
> I created a new branch named hotfix from master.
> I switched to the branch, changed 1 file.
>
> Now I want to see the diff from the both using
>
> git diff hotfix master
>
> I do not see any output (difference).
> When I do a git status I see my file with status mofified, not staged for
> commit.
Since you just created the branch, and did not add any content, there
is no difference to see. A branch is just a pointer to a commit. You
now have two pointers pointing at the same commit.
If you want to see the difference between your changes and the master
branch, you can omit the first reference:
git diff master
When you start adding commits to your hotfix branch, you will be able
to see the diff between that and master with the command that you
gave. However, your arguments may be in the reverse order than what
you expect. You want to specify master first because that is the
mainline branch (I presume).
When you have several commits on your hotfix branch, you can refer to
older commits to diff against. There are several ways to refer back,
but the simplest is to use a tilde '~' followed by a number to count
back. For example 'hotfix~1' refers to the parent commit on the
hotfix branch. There is a lot in the documentation[1], so take a look
there for more info.
Good luck.
_Mike
[1] https://git-scm.com/doc
> Also, I can see that I am working with the correct branch, hotfix
>
> What am I doing wrong?
>
> -fuz
On Wed, Oct 12, 2016 at 6:50 AM, <webmaster@peter-speer.de> wrote:
> Hi.
>
> I created a new branch named hotfix from master.
> I switched to the branch, changed 1 file.
>
> Now I want to see the diff from the both using
>
> git diff hotfix master
>
> I do not see any output (difference).
> When I do a git status I see my file with status mofified, not staged for
> commit.
> Also, I can see that I am working with the correct branch, hotfix
>
> What am I doing wrong?
>
> -fuz
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Jakub Narębski @ 2016-10-12 10:54 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, Junio C Hamano, Jeff King
In-Reply-To: <03278DA5-34B8-42F1-B52E-A42A3BCD5FB8@gmail.com>
W dniu 12.10.2016 o 00:26, Lars Schneider pisze:
>> On 09 Oct 2016, at 01:06, Jakub Narębski <jnareb@gmail.com> wrote:
>>
>> Part 1 of review, starting with the protocol v2 itself.
>>
>> W dniu 08.10.2016 o 13:25, larsxschneider@gmail.com pisze:
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>
>>> +upon checkin. By default these commands process only a single
>>> +blob and terminate. If a long running `process` filter is used
>>> +in place of `clean` and/or `smudge` filters, then Git can process
>>> +all blobs with a single filter command invocation for the entire
>>> +life of a single Git command, for example `git add --all`. See
>>> +section below for the description of the protocol used to
>>> +communicate with a `process` filter.
>>
>> I don't remember how this part looked like in previous versions
>> of this patch series, but "... is used in place of `clean` ..."
>> does not tell explicitly about the precedence of those
>> configuration variables. I think it should be stated explicitly
>> that `process` takes precedence over any `clean` and/or `smudge`
>> settings for the same `filter.<driver>` (regardless of whether
>> the long running `process` filter support "clean" and/or "smudge"
>> operations or not).
>
> This is stated explicitly later on. I moved it up here:
>
> "If a long running `process` filter is used
> in place of `clean` and/or `smudge` filters, then Git can process
> all blobs with a single filter command invocation for the entire
> life of a single Git command, for example `git add --all`. If a
> long running `process` filter is configured then it always takes
> precedence over a configured single blob filter. "
>
> OK?
Looks good to me.
I think this information about precedence between one-shot `clean`
and `smudge` filter driver configuration, and multi-file `process`
filter driver should be here for two reasons.
First, if one is interested in running filter, but do not want to
write one (he or she uses existing tool, for example one of
existing LFS solutions), one can skip the "Long Running Filter
Process" section. But one still needs to know if to remove or
comment out old `clean` and `smudge` config, or how to provide
fallback for older Git (if one uses the same configuration with
pre-process Git and Git including support for this feature).
Second, the configuration belongs, in my opinion, here. It is
not a part of long running filter protocol.
>>> +If the filter command (a string value) is defined via
>>> +`filter.<driver>.process` then Git can process all blobs with a
>>> +single filter invocation for the entire life of a single Git
>>> +command. This is achieved by using a packet format (pkt-line,
>>> +see technical/protocol-common.txt) based protocol over standard
>>> +input and standard output as follows. All packets, except for the
>>> +"*CONTENT" packets and the "0000" flush packet, are considered
>>> +text and therefore are terminated by a LF.
>>
>> Maybe s/standard input and output/\& of filter process,/ (that is,
>> add "... of filter process," to the third sentence in the above
>> paragraph).
>
> You mean "This is achieved by using a packet format (pkt-line,
> see technical/protocol-common.txt) based protocol over standard
> input and standard output of filter process as follows." ?
Yes.
> I think I like the original version better.
Well, I think it is better to err out on the side of being more
explicit.
>
>>> After the filter started
>>> Git sends a welcome message ("git-filter-client"), a list of
>>> supported protocol version numbers, and a flush packet. Git expects
>>> +to read a welcome response message ("git-filter-server") and exactly
>>> +one protocol version number from the previously sent list. All further
>>> +communication will be based on the selected version. The remaining
>>> +protocol description below documents "version=2". Please note that
>>> +"version=42" in the example below does not exist and is only there
>>> +to illustrate how the protocol would look like with more than one
>>> +version.
>>> +
>>> +After the version negotiation Git sends a list of all capabilities that
>>> +it supports and a flush packet. Git expects to read a list of desired
>>> +capabilities, which must be a subset of the supported capabilities list,
>>> +and a flush packet as response:
>>> +------------------------
>>> +packet: git> git-filter-client
>>> +packet: git> version=2
>>> +packet: git> version=42
>>> +packet: git> 0000
>>> +packet: git< git-filter-server
>>> +packet: git< version=2
>>> +packet: git> clean=true
>>> +packet: git> smudge=true
>>> +packet: git> not-yet-invented=true
>>> +packet: git> 0000
>>> +packet: git< clean=true
>>> +packet: git< smudge=true
>>> +packet: git< 0000
>>
>> WARNING: This example is different from description!!!
>
> Can you try to explain the difference more clearly? I read it multiple
> times and I think this is sound.
I'm sorry it was *my mistake*. I have read the example exchange wrong.
On the other hand that means that I have other comment, which I though
was addressed already in v10, namely that not all exchanges ends with
flush packet (inconsistency, and I think a bit of lack of extendability).
>> In example you have Git sending "git-filter-client" and list of supported
>> protocol versions, terminated with flush packet,
>
> Correct.
[thinking out loud]
And this serves as a 'canary' to detect single-shot driver mis-configured
to serve as multi-file filter driver.
>> then filter driver
>> process sends "git-filter-server", exactly one version, *AND* list of
>> supported capabilities in "<capability>=true" format, terminated with
>> flush packet.
>
> Correct. That's what I read in the text and in the example.
Actually, the text reads that filter driver sends two lines: a line with
magic signature "git-filter-server", and exactly one line with protocol
version "version=2", *WITHOUT* terminating flush packet.
The example reads the same, I have just missed change of prefix from
"git<" to "git>" (that is "<" to mark response from filter, to ">" to
mark signal from Git).
So the text and example agrees, just me (and now you) misread the
example ;-/
IMHO this exchange should be also terminated with a flush packet,
even if in protocol version 2 it is fixed length list, and doesn't
strictly need it.
First, it would make easier to implement the filter driver process.
You would need only one 'read until flush' helper function, and two
higher-level functions: one for handling metadata, one for handling
contents (where handling = sending or receiving). Currently first
data send from filter is a bit of special case: you need to send
two pkt-lines, not send this list of lines and terminate with flush.
Second, it would allow for additional possibilities for new versions
and extending protocol, either 3-part handshake (but now I think that
4-part is better, at least in some cases), or some other "early start"
extension. OTOH we could stuff this data in additional exchange
(assuming new protocol version), and unless the exchange data goes
through slow channel (e.g. network), it shouldn't matter for the
latency that we have one more exchange.
Third, as we can see first from my error, then from yours, it would
make it easier to debug the protocol...
>>
>> In description above the example you have 4-part handshake, not 3-part;
>> the filter is described to send list of supported capabilities last
>> (a subset of what Git command supports).
>
> Part 1: Git sends a welcome message...
> Part 2: Git expects to read a welcome response message...
> Part 3: After the version negotiation Git sends a list of all capabilities...
> Part 4: Git expects to read a list of desired capabilities...
>
> I think example and text match, no?
Yes, it does; as I have said already, I have misread the example.
Anyway, in some cases 4-way handshake, where Git sends list of
supported capabilities first, is better. If the protocol has
to prepare something for each of capabilities, and perhaps check
those preparation status, it can do it after Git sends what it
could need, and before it sends what it does support.
Though it looks a bit strange that client (as Git is client here)
sends its capabilities first...
>> Moreover in the example in
>> previous version at least as far as v8 of this series, the response
>> from filter driver was fixed length list of two lines: magic string
>> "git-filter-server" and exactly one line with protocol version; this
>> part was *not* terminated with a flush packet (complicating code of
>> filter driver program a bit, I think).
>>
>> I think this version of protocol is *better*, just the text needs to
>> be updated to match. I wanted to propose something like this in v9,...
>
> I didn't change that behavior since v8:
> packet: git< git-filter-server
> packet: git< version=2
Right.
>> By the way, now I look at it, the argument for using the
>> "<capability>=true" format instead of "capability=<capability>"
>> (or "supported-command=<capability>") is weak. The argument for
>> using "<variable>=<value>" to make it easier to implement parsing
>> is sound, but the argument for "<capability>=true" is weak.
>>
>> The argument was that with "<capability>=true" one can simply
>> parse metadata into hash / dictionary / hashmap, and choose
>> response based on that. Hash / hashmap / associative array
>> needs different keys, so the reasoning went for "<capability>=true"
>> over "capability=<capability>"... but the filter process still
>> needs to handle lines with repeating keys, namely "version=<N>"
>> lines!
>>
>> So the argument doesn't hold water IMVHO, and we can choose
>> version which reads better / is more natural.
>
> I have to agree that "capability=<capability>" might read a
> little bit nicer. However, Peff suggested "<capability>=true"
> as his preference and this is absolutely OK with me.
From what I remember it was Peff stating that he thinks "<foo>=true"
is easier for parsing (it is, but we still need to support the harder
way parsing anyway), and offered that "<foo>" is good enough (if less
consistent).
> I am happy to change that if a second reviewer shares your
> opinion.
Also, with "capability=<foo>" we can be more self descriptive,
for example "supported-command=<foo>"; though "capability" is good
enough for me.
For example
packet: git> wants=clean
packet: git> wants=smudge
packet: git> wants=size
packet: git> 0000
packet: git< supports=clean
packet: git< supports=smudge
packet: git< 0000
Though coming up with good names is hard; and as I said "capability"
is good enough; OTOH with "smudge=true" etc. we don't need to come
up with good name at all... though I wonder if it is a good thing `\_o,_/
>>> +Afterwards Git sends a list of "key=value" pairs terminated with
>>> +a flush packet. The list will contain at least the filter command
>>> +(based on the supported capabilities) and the pathname of the file
>>> +to filter relative to the repository root. Right after these packets
>>
>> I think you meant here "right after the flush packet", isn't it?
>> It would be more explicit.
>
> I feel "right after these packets" reads better, but I agree that your
> version is more explicit. I will change it.
Thanks. That doesn't matter much, but it matters.
Though it could go either way.
>>> Finally, a
>>> +second list of "key=value" pairs terminated with a flush packet
>>> +is expected. The filter can change the status in the second list.
>>
>> I would add here, to be more explicit:
>>
>> This second list of "key=value" pairs may be empty, and usually
>> would be if there is nothing wrong with response or filter; the
>> terminating flush packet must be here regardless.
>>
>> Or something like that. The above proposal could be certainly
>> improved.
>
> How about this:
>
> "Finally, a
> second list of "key=value" pairs terminated with a flush packet
> is expected. The filter can change the status in the second list
> or keep the status as is with an empty list. Please note that the
> empty list must be terminated with a flush packet regardless."
>
> TBH I like the original version and I wonder if the new version
> is redundant?!
I'm a bit unsure. Original reads better and is shorter; the new
proposal is more explicit, but also more repetitive and longer.
>>> +------------------------
>>> +packet: git< status=success
>>> +packet: git< 0000
>>> +packet: git< SMUDGED_CONTENT
>>> +packet: git< 0000
>>> +packet: git< 0000 # empty list, keep "status=success" unchanged!
>>
>> All right, looks good. Is this exclamation mark "!" necessary / wanted?
>
> Yes, to draw the attention towards the two flushes.
O.K. though shouldn't it be after "empty list", then?
>>> +------------------------
>>> +
>>> +If the result content is empty then the filter is expected to respond
>>> +with a "success" status and an empty list.
>>
>> Actually, it is empty content, not empty list; that is response (filter
>> output) composed entirely of flush packet.
>
> Correct!
>
> "If the result content is empty then the filter is expected to respond
> with a "success" status and a flush packet to signal the empty content."
>
> Better?
Better, I think.
>>
>>> +------------------------
>>> +packet: git< status=error
>>> +packet: git< 0000
>>> +------------------------
>>> +
>>> +If the filter experiences an error during processing, then it can
>>> +send the status "error" after the content was (partially or
>>> +completely) sent. Depending on the `filter.<driver>.required` flag
>>> +Git will interpret that as error but it will not stop or restart the
>>> +filter process.
>>
>> Errr... this is literal repetition. You need to decide whether to
>> put it before example, or after example. Or maybe split it.
>
> Agreed. I removed the repetition and changed the previous paragraph
> to:
>
> "In case the filter cannot or does not want to process the content,
> it is expected to respond with an "error" status. Git will handle
> the "error" status according to the `filter.<driver>.required` flag
> but it will not stop or restart the filter process."
All right, I think.
>>> +------------------------
>>> +packet: git< status=success
>>> +packet: git< 0000
>>> +packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT
>>> +packet: git< 0000
>>> +packet: git< status=error
>>> +packet: git< 0000
>>> +------------------------
>>> +
>>> +If the filter dies during the communication or does not adhere to
>>> +the protocol then Git will stop the filter process and restart it
>>> +with the next file that needs to be processed. Depending on the
>>> +`filter.<driver>.required` flag Git will interpret that as error.
>>
>> Uhh... until now the order was explanation, then example. From the
>> duplicated description above, it is now first example, then
>> description. Consistency would be good.
>
> OK, I moved that down after the EOF exit explanation.
Good.
>>> +The error handling for all cases above mimic the behavior of
>>> +the `filter.<driver>.clean` / `filter.<driver>.smudge` error
>>> +handling.
>>
>> You have "error handling" repeated here.
>
> True. That might not be nice from a stylistic point of view but it is
> precise, no?
All right, though you could also write it as "mimic what the ...
do in those cases"; I'm not sure if its better or worse.
>>> +------------------------
>>> +packet: git< status=abort
>>> +packet: git< 0000
>>> +------------------------
>>> +
>>> +After the filter has processed a blob it is expected to wait for
>>> +the next "key=value" list containing a command. Git will close
>>> +the command pipe on exit. The filter is expected to detect EOF
>>> +and exit gracefully on its own.
>>
>> Any "kill filter" solutions should probably be put here.
>
> Agreed.
>
>> I guess
>> that filter exiting means EOF on its standard output when read
>> by Git command, isn't it?
>
> Yes, but at this point Git is not listening anymore.
I think it might be good idea to have here the information about
what filter process should do if it needs maybe lengthy closing
process, to not hold/stop Git command or to not be killed.
>>> +If you develop your own long running filter
>>> +process then the `GIT_TRACE_PACKET` environment variables can be
>>> +very helpful for debugging (see linkgit:git[1]).
>>
>> s/environment variables/environment variable/ - there is only
>> one GIT_TRACE_PACKET. Unless you wanted to write about GIT_TRACE?
>
> Agreed.
>
>
> Thanks for the review,
You are welcome.
Thanks for working on this series,
--
Jakub Narębski
^ permalink raw reply
* git diff
From: webmaster @ 2016-10-12 10:50 UTC (permalink / raw)
To: git
Hi.
I created a new branch named hotfix from master.
I switched to the branch, changed 1 file.
Now I want to see the diff from the both using
git diff hotfix master
I do not see any output (difference).
When I do a git status I see my file with status mofified, not staged for
commit.
Also, I can see that I am working with the correct branch, hotfix
What am I doing wrong?
-fuz
^ permalink raw reply
* Bug with git merge-base and a packed ref
From: Stepan Kasal @ 2016-10-12 10:37 UTC (permalink / raw)
To: git
Hello,
first, I observed a bug with git pull --rebase:
if the remote branch got rebased and the loval branch was updated,
pull tried to rebase the whole branch, not the local increment.
A reproducer would look like that
# in repo1:
git checkout tmp
cd ..
git clone repo1 repo2
cd repo1
git rebase elsewhere tmp
cd ../repo2
# edit
git commit -a -m 'Another commit'
git pull -r
The last command performs something like
git rebase new-origin/tmp
instead of
git rebase --onto new-origin/tmp old-origin/tmp
I'm using git version 2.10.1.windows.1
I tried to debug the issue:
I found that the bug happens only at the very first pull after clone.
I was able to reproduce it with git-pull.sh
The problem seems to be that command
git merge-base --fork-point refs/remotes/origin/tmp refs/heads/tmp
returns nothing, because the refs are packed.
Could you please fix merge-base so that it understands packed refs?
Thanks,
Stepan
^ permalink raw reply
* RE: git merge deletes my changes
From: Eduard Egorov @ 2016-10-12 5:51 UTC (permalink / raw)
To: 'Jakub Narębski'
Cc: 'Paul Smith', 'git@vger.kernel.org',
'Jeff King'
In-Reply-To: <94ff5fb3-6957-8983-4aa7-e1d5e2692e82@gmail.com>
Hello Jakub,
Thank you for addition. Eventually, I've merged your explanations into single answer post (http://stackoverflow.com/questions/39954265/git-merge-s-subtree-works-incorrectly/ ). I hope this will prevent other confused people from disturbing you by similar emails on this mailing list.
This is another time I can evidence the power and flexibility the git provides, thank you all for your great work!
With best regards
Eduard Egorov
-----Original Message-----
From: Jakub Narębski [mailto:jnareb@gmail.com]
Sent: Tuesday, October 11, 2016 6:57 PM
To: Paul Smith; Eduard Egorov; 'git@vger.kernel.org'
Subject: Re: git merge deletes my changes
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: [PATCH v2 2/2] Feature Request: user defined suffix for temp files created by git-mergetool
From: Josef Ridky @ 2016-10-12 8:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7f9lmmol.fsf@gitster.mtv.corp.google.com>
This is update of the second variant for request to add option to change
suffix of name of temporary files generated by git mergetool. This
change is requested for cases, when is git mergetool used for local
comparison between two version of same package during package rebase.
Signed-off-by: Josef Ridky <jridky@redhat.com>
---
Documentation/git-mergetool.txt | 22 ++++++++++++++-
git-mergetool.sh | 60 +++++++++++++++++++++++++++++++++++++----
2 files changed, 76 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index e846c2e..a0466ac 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -8,7 +8,7 @@ git-mergetool - Run merge conflict resolution tools to resolve merge conflicts
SYNOPSIS
--------
[verse]
-'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [<file>...]
+'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [--local=<name>] [--remote=<name>] [--backup=<name>] [--base=<name>] [<file>...]
DESCRIPTION
-----------
@@ -79,6 +79,26 @@ success of the resolution after the custom tool has exited.
Prompt before each invocation of the merge resolution program
to give the user a chance to skip the path.
+--local=<name>::
+ Use string from <name> as part of suffix of name of temporary
+ file (local) for merging. If not set, default value is used.
+ Default suffix is LOCAL.
+
+--remote=<name>::
+ Use string from <name> as part of suffix of name of temporary
+ file (remote) for merging. If not set, default value is used.
+ Default suffix is REMOTE.
+
+--backup=<name>::
+ Use string from <name> as part of suffix of name of temporary
+ file (backup) for merging. If not set, default value is used.
+ Default suffix is BACKUP.
+
+--base=<name>::
+ Use string from <name> as part of suffix of name of temporary
+ file (base) for merging. If not set, default value is used.
+ Default suffix is BASE.
+
TEMPORARY FILES
---------------
`git mergetool` creates `*.orig` backup files while resolving merges.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index bf86270..ed9ba82 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,7 +8,7 @@
# at the discretion of Junio C Hamano.
#
-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [--local=name] [--remote=name] [--backup=name] [--base=name] [file to merge] ...'
SUBDIRECTORY_OK=Yes
NONGIT_OK=Yes
OPTIONS_SPEC=
@@ -16,6 +16,13 @@ TOOL_MODE=merge
. git-sh-setup
. git-mergetool--lib
+# Can be changed by user
+LOCAL_NAME='LOCAL'
+BASE_NAME='BASE'
+BACKUP_NAME='BACKUP'
+REMOTE_NAME='REMOTE'
+
+
# Returns true if the mode reflects a symlink
is_symlink () {
test "$1" = 120000
@@ -271,10 +278,10 @@ merge_file () {
BASE=${BASE##*/}
fi
- BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
- LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
- REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
- BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
+ BACKUP="$MERGETOOL_TMPDIR/${BASE}_${BACKUP_NAME}_$$$ext"
+ LOCAL="$MERGETOOL_TMPDIR/${BASE}_${LOCAL_NAME}_$$$ext"
+ REMOTE="$MERGETOOL_TMPDIR/${BASE}_${REMOTE_NAME}_$$$ext"
+ BASE="$MERGETOOL_TMPDIR/${BASE}_${BASE_NAME}_$$$ext"
base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}')
local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}')
@@ -396,6 +403,18 @@ do
--prompt)
prompt=true
;;
+ --local=*)
+ LOCAL_NAME=${1#--local=}
+ ;;
+ --remote=*)
+ REMOTE_NAME=${1#--remote=}
+ ;;
+ --base=*)
+ BASE_NAME=${1#--base=}
+ ;;
+ --backup=*)
+ BACKUP_NAME=${1#--backup=}
+ ;;
--)
shift
break
@@ -410,6 +429,37 @@ do
shift
done
+# sanity check after parsing command line
+case "" in
+"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
+ die "You cannot set any of --local/remote/base/backup to empty."
+ ;;
+esac
+
+case "$LOCAL_NAME" in
+"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
+ die "You cannot set any of --remote/base/backup to same as --local."
+ ;;
+esac
+
+case "$REMOTE_NAME" in
+"$LOCAL_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
+ die "You cannot set any of --local/base/backup to same as --remote."
+ ;;
+esac
+
+case "$BASE_NAME" in
+"$LOCAL_NAME"|"$REMOTE_NAME"|"$BACKUP_NAME")
+ die "You cannot set any of --local/remote/backup to same as --base."
+ ;;
+esac
+
+case "$BACKUP_NAME" in
+"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME")
+ die "You cannot set any of --local/remote/base to same as --backup."
+ ;;
+esac
+
prompt_after_failed_merge () {
while true
do
--
2.7.4
^ permalink raw reply related
* Re: Formatting problem send_mail in version 2.10.0
From: Matthieu Moy @ 2016-10-12 7:36 UTC (permalink / raw)
To: Larry Finger; +Cc: Jeff King, Mathieu Lienard--Mayor, Remi Lespinet, git
In-Reply-To: <b8f93bf9-bfa5-2405-437e-6bf9abf77c87@lwfinger.net>
Larry Finger <Larry.Finger@lwfinger.net> writes:
> On 10/11/2016 11:18 AM, Matthieu Moy wrote:
>> 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?
>
> This format is used in a patch for the kernel. When the patch is
> merged into mainline, stable@vger.kernel.org gets sent an E-mail with
> a copy of the original patch. Maintainers of the indicated systems
> then merge the patch with their stable version.
Sorry, but this does not answer my question. I'll rephrase: when
people behind stable@vger.kernel.org get the message, how do they know
which version of the kernel they should apply it to?
> I do not want it in the body of the message. I just want to pass a
> hint to the stable maintainer(s).
If it's not in the body of the message, then where is it?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 1/5] trailer: use singly-linked list, not doubly
From: Junio C Hamano @ 2016-10-12 6:24 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, christian.couder
In-Reply-To: <8e12e0954f0a23d7c7905c58a3f7d8084d9338be.1476232683.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> Use singly-linked lists (instead of doubly-linked lists) in trailer to
> keep track of arguments (whether implicit from configuration or explicit
> from the command line) and trailer items.
>
> This change significantly reduces the code length and simplifies the code.
> There are now fewer pointers to be manipulated, but most trailer
> manipulations now require seeking from beginning to end, so there might
> be a slight net decrease in performance; however the number of trailers
> is usually small (10 to 15 at the most) so this should not cause a big
> impact.
It is overall a very good change, but can you split this into two
independent patches? s/struct trailer_item/const &/ sprinkled all
over the place is more or less unrelated change and it is very
distracting to see the primary change of the way lists are handled.
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-12 6:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <xmqqvawy5c4i.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 11, 2016 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> I think this patch is the most interesting patch, so I'll refrain from
>> resending the other 27 patches, though I have adressed the review comments
>> locally. I'll resend everything once we are in agreement for this one.
>
> What is the primary purpose of this patch? Is it to prepare callers
> so that the way they interact with the attr subsystem will not have to
> change when they become threaded and the attr subsystem becomes
> thread ready?
>
> I am not sure if the updates to the callers fulfill that purpose.
> For example, look at this hunk.
>
>> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
>> struct archiver_args *args = c->args;
>> write_archive_entry_fn_t write_entry = c->write_entry;
>> static struct git_attr_check *check;
>> + static struct git_attr_result result;
>
> As we discussed, this caller, even when threaded, will always want
> to ask for a fixed two attributes, so "check" being static and
> shared across threads is perfectly fine. But we do not want to see
> "result" shared, do we?
Well all of the hunks in the patch are not threaded, so they
don't follow a threading pattern, but the static pattern to not be
more expensive than needed.
>
>> const char *path_without_prefix;
>> int err;
>>
>> @@ -124,12 +125,15 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
>> strbuf_addch(&path, '/');
>> path_without_prefix = path.buf + args->baselen;
>>
>> - if (!check)
>> - check = git_attr_check_initl("export-ignore", "export-subst", NULL);
>> - if (!git_check_attr(path_without_prefix, check)) {
>> - if (ATTR_TRUE(check->check[0].value))
>> + if (!check) {
>> + git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
>> + git_attr_result_init(&result, check);
>> + }
>
> Are we assuming that storing and checking of a single pointer is
> atomic? I would not expose that assumption to the callers. On a
> platform where that assumption holds, "if check is not NULL,
> somebody must have done it already, so return without doing nothing"
> can be the first thing git_attr_check_initl()'s implementation does,
> though. Or it may not hold anywhere without some barriers. All
> that implementation details should be hidden inside _initl()'s
> implementation. So this caller should instead just do an
> unconditional:
>
> git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
>
> Also, as "result" should be per running thread, hence non-static,
> and because we do not want repeated heap allocations and releases
> but luckily most callers _know_ not just how many but what exact
> attributes they are interested in (I think there are only two
> callers that do not know it; check-all-attrs one, and your pathspec
> magic one that does not exist at this point in the series), I would
> think it is much more preferrable to allow the caller to prepare an
> on-stack array and call it "initialized already".
>
> In other words, ideally, I think this part of the patch should
> rather read like this:
>
> static struct git_attr_check *check;
> struct git_attr_result result[2];
>
> ...
> git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
> if (!git_check_attr(path_without_prefix, check, result)) {
> ... use result[0] and result[1] ...
>
> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
> and extra parameter to git_check_attr() so that the function can
> make sure it matches (or exceeds) check->nr.
That seems tempting from a callers perspective; I'll look into that.
^ permalink raw reply
* Re: Make `git fetch --all` parallel?
From: Stefan Beller @ 2016-10-12 6:47 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ram Rachum, git@vger.kernel.org
In-Reply-To: <20161012015224.g2eb24jexepeewob@sigill.intra.peff.net>
On Tue, Oct 11, 2016 at 6:52 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 11, 2016 at 09:34:28PM -0400, Jeff King wrote:
>
>> > Ok, time to present data... Let's assume a degenerate case first:
>> > "up-to-date with all remotes" because that is easy to reproduce.
>> >
>> > I have 14 remotes currently:
>> >
>> > $ time git fetch --all
>> > real 0m18.016s
>> > user 0m2.027s
>> > sys 0m1.235s
>> >
>> > $ time git config --get-regexp remote.*.url |awk '{print $2}' |xargs
>> > -P 14 -I % git fetch %
>> > real 0m5.168s
>> > user 0m2.312s
>> > sys 0m1.167s
>>
>> So first, thank you (and Ævar) for providing real numbers. It's clear
>> that I was talking nonsense.
>>
>> Second, I wonder where all that time is going. Clearly there's an
>> end-to-end latency issue, but I'm not sure where it is. Is it startup
>> time for git-fetch? Is it in getting and processing the ref
>> advertisement from the other side? What I'm wondering is if there are
>> opportunities to speed up the serial case (but nobody really cared
>> before because it doesn't matter unless you're doing 14 of them back to
>> back).
>
> Hmm. I think it really might be just network latency. Here's my fetch
> time:
>
> $ git config remote.origin.url
> git://github.com/gitster/git.git
>
> $ time git fetch origin
> real 0m0.183s
> user 0m0.072s
> sys 0m0.008s
>
> 14 of those in a row shouldn't take more than about 2.5 seconds, which
> is still twice as fast as your parallel case. So what's going on?
>
> One is that I live about a hundred miles from GitHub's data center, and
> my ping time there is ~13ms. The other side of the country, let alone
> Europe, is going to be noticeably slower just for the TCP handshake.
>
> The second is that git:// is really cheap and simple. git-over-ssh is
> over twice as slow:
>
> $ time git fetch git@github.com:gitster/git
> ...
> real 0m0.432s
> user 0m0.100s
> sys 0m0.032s
>
> HTTP fares better than I would have thought, but is also slower:
>
> $ time git fetch https://github.com/gitster/git
> ...
> real 0m0.258s
> user 0m0.080s
> sys 0m0.032s
>
> -Peff
Well 9/14 are https for me, the rest is git://
Also 9/14 (but a different set) is github, the rest is
either internal or kernel.org.
Fetching from github (https) is only 0.9s from here
(SF bay area, I'm not in Europe any more ;) )
I would have expected to have a speedup
of roughly 2 + latency gains. Factor 2 because
in the current state of affairs either the client or the
remote is working, i.e. the other sie is idle/waiting, so
factor 2 seemed reasonable (and ofc the latency), so I
was a bit surprised to see a higher yield.
^ permalink raw reply
* Re: [PATCH 5/5] trailer: support values folded to multiple lines
From: Junio C Hamano @ 2016-10-12 6:23 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, christian.couder
In-Reply-To: <4b8616732b719ede04b90c87ab240c29b4e3a0bb.1476232683.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> Currently, interpret-trailers requires that a trailer be only on 1 line.
> For example:
>
> a: first line
> second line
>
> would be interpreted as one trailer line followed by one non-trailer line.
>
> Make interpret-trailers support RFC 822-style folding, treating those
> lines as one logical trailer.
Let's see how the code handles one minor detail when we see 822
folding, namely, "what happens to the leading whitespace that signals
the beginning of the second and subsequent lines?".
> diff --git a/trailer.c b/trailer.c
> index 97e96a9..907baa0 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -31,7 +31,7 @@ struct trailer_item {
> * (excluding the terminating newline) and token is NULL.
> */
> char *token;
> - char *value;
> + struct strbuf value;
> };
Is the length of value very frequently used once the list of trailer
lines are fully parsed? If not, I'd rather not to have "struct
strbuf" in a long-living structure like this one and instead prefer
keeping it a simple and stupid "char *value".
Yes, I know the existing code in trailers overuses strbuf when there
is no need, primarily because it uses the lazy "split into an array
of strbufs" function. We shouldn't make it worse.
> @@ -767,16 +773,24 @@ static int process_input_file(FILE *outfile,
>
> /* Parse trailer lines */
> for (i = trailer_start; i < trailer_end; i++) {
> + if (last && isspace(lines[i]->buf[0])) {
It is convenient if "value" is a strbuf to do this,
> + /* continuation line of the last trailer item */
> + strbuf_addch(&last->value, '\n');
> + strbuf_addbuf(&last->value, lines[i]);
> + strbuf_strip_suffix(&last->value, "\n");
but it is easy to introduce a temporary strbuf in this scope and use
it only to create the final value and detach it to last->value, i.e.
if (last && isspace(*lines[i]->buf)) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "%s\n%s", last->value, lines[i]->buf);
strbuf_strip_suffix(&buf, "\n");
free(last->value);
last->value = strbuf_detach(&buf, NULL);
By the way, I now see that the code handles the "minor detail" to
keep the leading whitespace, which is good.
Thanks.
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 6:12 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill
In-Reply-To: <20161011235951.8358-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> I think this patch is the most interesting patch, so I'll refrain from
> resending the other 27 patches, though I have adressed the review comments
> locally. I'll resend everything once we are in agreement for this one.
What is the primary purpose of this patch? Is it to prepare callers
so that the way they interact with the attr subsystem will not have to
change when they become threaded and the attr subsystem becomes
thread ready?
I am not sure if the updates to the callers fulfill that purpose.
For example, look at this hunk.
> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
> struct archiver_args *args = c->args;
> write_archive_entry_fn_t write_entry = c->write_entry;
> static struct git_attr_check *check;
> + static struct git_attr_result result;
As we discussed, this caller, even when threaded, will always want
to ask for a fixed two attributes, so "check" being static and
shared across threads is perfectly fine. But we do not want to see
"result" shared, do we?
> const char *path_without_prefix;
> int err;
>
> @@ -124,12 +125,15 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
> strbuf_addch(&path, '/');
> path_without_prefix = path.buf + args->baselen;
>
> - if (!check)
> - check = git_attr_check_initl("export-ignore", "export-subst", NULL);
> - if (!git_check_attr(path_without_prefix, check)) {
> - if (ATTR_TRUE(check->check[0].value))
> + if (!check) {
> + git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
> + git_attr_result_init(&result, check);
> + }
Are we assuming that storing and checking of a single pointer is
atomic? I would not expose that assumption to the callers. On a
platform where that assumption holds, "if check is not NULL,
somebody must have done it already, so return without doing nothing"
can be the first thing git_attr_check_initl()'s implementation does,
though. Or it may not hold anywhere without some barriers. All
that implementation details should be hidden inside _initl()'s
implementation. So this caller should instead just do an
unconditional:
git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
Also, as "result" should be per running thread, hence non-static,
and because we do not want repeated heap allocations and releases
but luckily most callers _know_ not just how many but what exact
attributes they are interested in (I think there are only two
callers that do not know it; check-all-attrs one, and your pathspec
magic one that does not exist at this point in the series), I would
think it is much more preferrable to allow the caller to prepare an
on-stack array and call it "initialized already".
In other words, ideally, I think this part of the patch should
rather read like this:
static struct git_attr_check *check;
struct git_attr_result result[2];
...
git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
if (!git_check_attr(path_without_prefix, check, result)) {
... use result[0] and result[1] ...
For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
and extra parameter to git_check_attr() so that the function can
make sure it matches (or exceeds) check->nr.
^ 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