Git development
 help / color / mirror / Atom feed
* [PATCH] submodules: update documentaion for submodule branches
From: Brandon Williams @ 2016-10-19 20:42 UTC (permalink / raw)
  To: git; +Cc: sbeller, Brandon Williams

Update the documentaion for the the special value `.` to indicate that
it signifies that the tracking branch in the submodule should be the
same as the current branch in the superproject.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git-submodule.txt | 4 +++-
 Documentation/gitmodules.txt    | 7 +++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index bf3bb37..d841573 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -259,7 +259,9 @@ OPTIONS
 --branch::
 	Branch of repository to add as submodule.
 	The name of the branch is recorded as `submodule.<name>.branch` in
-	`.gitmodules` for `update --remote`.
+	`.gitmodules` for `update --remote`.  A special value of `.` is used to
+	indicate that the name of the branch in the submodule should be the
+	same name as the current branch in the current repository.
 
 -f::
 --force::
diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
index 10dcc08..8f7c50f 100644
--- a/Documentation/gitmodules.txt
+++ b/Documentation/gitmodules.txt
@@ -50,8 +50,11 @@ submodule.<name>.update::
 
 submodule.<name>.branch::
 	A remote branch name for tracking updates in the upstream submodule.
-	If the option is not specified, it defaults to 'master'.  See the
-	`--remote` documentation in linkgit:git-submodule[1] for details.
+	If the option is not specified, it defaults to 'master'.  A special
+	value of `.` is used to indicate that the name of the branch in the
+	submodule should be the same name as the current branch in the
+	current repository.  See the `--remote` documentation in
+	linkgit:git-submodule[1] for details.
 
 submodule.<name>.fetchRecurseSubmodules::
 	This option can be used to control recursive fetching of this
-- 
2.10.1


^ permalink raw reply related

* Re: Problems with "git svn clone"
From: Eric Wong @ 2016-10-19 20:41 UTC (permalink / raw)
  To: K Richard Pixley; +Cc: git
In-Reply-To: <500ef24a-55f9-1b38-52bb-17347ebaabcd@zebra.com>

K Richard Pixley <cnp637@zebra.com> wrote:
> error: git-svn died of signal 11
> 
> This seems awfully early and blatant for a core dump.  What can I do to
> get this running?

Can you show us a backtrace?  Thanks.

> Initially discovered on git-2.7.4, (ubuntu-16.04), but also reproduced
> on freshly built top of tree git-2.10.1.445.g3cdd5d1.

This could be a problem with the SVN Perl libraries, and should
be fixed in newer versions (not sure if it's made it to distros,
yet):

https://public-inbox.org/git/0BCA1E695085C645B9CD4A27DD59F6FA39AAD5CF@GBWGCEUHUBD0101.rbsres07.net/T/

Seems like it is fixed in latest Debian, maybe it needs to trickle
into Ubuntu: https://bugs.debian.org/780246

^ permalink raw reply

* [PATCH 2/2] tag: send fully qualified refnames to verify_tag_and_format
From: Jeff King @ 2016-10-19 20:39 UTC (permalink / raw)
  To: Santiago Torres; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161019203520.zevkb75at2nrogdm@sigill.intra.peff.net>

The ref-filter code generally expects to see fully qualified
refs, so that things like "%(refname)" and "%(refname:short)"
work as expected. We can do so easily from git-tag, which
always works with refnames in the refs/tags namespace. As a
bonus, we can drop the "kind" parameter from
pretty_print_ref() and just deduce it automatically.

Unfortunately, things are not so simple for verify-tag,
which takes an arbitrary sha1 expression. It has no clue if
a refname as used or not, and whether it was in the
refs/tags namespace.

In an ideal world, get_sha1_with_context() would optionally
tell us about any refs we resolved while it was working, and
we could just feed that refname (and then in cases where we
didn't use a ref at all, like a bare sha1, we could fallback
to just showing the sha1 name the user gave us).

Signed-off-by: Jeff King <peff@peff.net>
---
I think you'd really just squash the various bits of this into your
series at the right spots, though I don't mind it on top, either.

The big question is to what degree we should care about the verify-tag
case. I don't think it's any worse off with this change than it is with
your series (its "kind" becomes "OTHER", but I don't think that is
actually used for display at all; the name remains the same). I'd be OK
with leaving it like this, as a known bug, until get_sha1_with_context()
learns to tell us about the ref. It's an unhandled corner case in a
brand-new feature, not a regression in an existing one.

 builtin/tag.c | 2 +-
 ref-filter.c  | 4 ++--
 ref-filter.h  | 6 +++++-
 tag.c         | 2 +-
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 49aeb50..18eab7e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -114,7 +114,7 @@ static int verify_tag(const char *name, const char *ref,
 	if (fmt_pretty)
 		flags = GPG_VERIFY_QUIET;
 
-	return verify_and_format_tag(sha1, name, fmt_pretty, flags);
+	return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
 }
 
 static int do_sign(struct strbuf *buffer)
diff --git a/ref-filter.c b/ref-filter.c
index 77ec9de..74da17a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1643,11 +1643,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 }
 
 void pretty_print_ref(const char *name, const unsigned char *sha1,
-		const char *format, unsigned kind)
+		      const char *format)
 {
 	struct ref_array_item *ref_item;
 	ref_item = new_ref_array_item(name, sha1, 0);
-	ref_item->kind = kind;
+	ref_item->kind = ref_kind_from_refname(name);
 	show_ref_array_item(ref_item, format, 0);
 	free_array_item(ref_item);
 }
diff --git a/ref-filter.h b/ref-filter.h
index 3d23090..fed2f5e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -107,7 +107,11 @@ struct ref_sorting *ref_default_sorting(void);
 /*  Function to parse --merged and --no-merged options */
 int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
 
+/*
+ * Print a single ref, outside of any ref-filter. Note that the
+ * name must be a fully qualified refname.
+ */
 void pretty_print_ref(const char *name, const unsigned char *sha1,
-		const char *format, unsigned kind);
+		      const char *format);
 
 #endif /*  REF_FILTER_H  */
diff --git a/tag.c b/tag.c
index d3512c0..d5a7cfb 100644
--- a/tag.c
+++ b/tag.c
@@ -62,7 +62,7 @@ int verify_and_format_tag(const unsigned char *sha1, const char *name,
 	free(buf);
 
 	if (fmt_pretty)
-		pretty_print_ref(name, sha1, fmt_pretty, FILTER_REFS_TAGS);
+		pretty_print_ref(name, sha1, fmt_pretty);
 
 	return ret;
 }
-- 
2.10.1.619.g16351a7

^ permalink raw reply related

* Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item
From: Jeff King @ 2016-10-19 20:35 UTC (permalink / raw)
  To: Santiago Torres; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161019170733.ey3d53miykn5t5cq@LykOS.localdomain>

On Wed, Oct 19, 2016 at 01:07:34PM -0400, Santiago Torres wrote:

> > I guess that may complicate things for the caller you add in this
> > series, which may not have a fully-qualified refname (which is obviously
> > how filter_ref_kind() figures it out). I'd argue that is a bug, though,
> > as things like "%(refname)" are generally expected to print out the
> > fully refname ("git tag --format=%(refname)" does so, and you can use
> > "%(refname:short)" if you want the shorter part).
> 
> Hmm, I hadn't actually noticed that. Do you have any suggestions in how to
> address this?
> 
> In general this feels like a consequence of disambiguating .git/tags/*
> within builtin/tag.c rather than letting ref-filter figure it out.

The partial solution would look like something below. It's not too bad
because git-tag always knows that it's working a ref in the refs/tags
namespace (and we don't even have to qualify it ourselves,
for_each_tag_name already does it for us).

But verify-tag feeds arbitrary sha1 expressions. See the notes in the
second patch.

  [1/2]: ref-filter: split ref_kind_from_filter
  [2/2]: tag: send fully qualified refnames to verify_tag_and_format

 builtin/tag.c |  2 +-
 ref-filter.c  | 21 +++++++++++++--------
 ref-filter.h  |  6 +++++-
 tag.c         |  2 +-
 4 files changed, 20 insertions(+), 11 deletions(-)

-Peff

^ permalink raw reply

* [PATCH 1/2] ref-filter: split ref_kind_from_filter
From: Jeff King @ 2016-10-19 20:35 UTC (permalink / raw)
  To: Santiago Torres; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161019203520.zevkb75at2nrogdm@sigill.intra.peff.net>

This function does two things: if we know we are filtering
only a certain kind of ref, then we can immediately know
that we have that kind. If not, then we compute the kind
from the fully-qualified refname. The latter half is useful
for other callers; let's split it out.

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index cfbcd73..77ec9de 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1329,7 +1329,7 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 	return ref;
 }
 
-static int filter_ref_kind(struct ref_filter *filter, const char *refname)
+static int ref_kind_from_refname(const char *refname)
 {
 	unsigned int i;
 
@@ -1342,11 +1342,7 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)
 		{ "refs/tags/", FILTER_REFS_TAGS}
 	};
 
-	if (filter->kind == FILTER_REFS_BRANCHES ||
-	    filter->kind == FILTER_REFS_REMOTES ||
-	    filter->kind == FILTER_REFS_TAGS)
-		return filter->kind;
-	else if (!strcmp(refname, "HEAD"))
+	if (!strcmp(refname, "HEAD"))
 		return FILTER_REFS_DETACHED_HEAD;
 
 	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
@@ -1357,6 +1353,15 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)
 	return FILTER_REFS_OTHERS;
 }
 
+static int filter_ref_kind(struct ref_filter *filter, const char *refname)
+{
+	if (filter->kind == FILTER_REFS_BRANCHES ||
+	    filter->kind == FILTER_REFS_REMOTES ||
+	    filter->kind == FILTER_REFS_TAGS)
+		return filter->kind;
+	return ref_kind_from_refname(refname);
+}
+
 /*
  * A call-back given to for_each_ref().  Filter refs and keep them for
  * later object processing.
-- 
2.10.1.619.g16351a7


^ permalink raw reply related

* Re: What's cooking in git.git (Oct 2016, #04; Mon, 17)
From: brian m. carlson @ 2016-10-19 20:28 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Lars Schneider, git
In-Reply-To: <20161019074648.nuk2c2mpvmedjrhb@sigill.intra.peff.net>

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

On Wed, Oct 19, 2016 at 03:46:48AM -0400, Jeff King wrote:
> FWIW, I gave it a fairly thorough read-over (something I'd been meaning
> to do for quite a while, but kept never quite getting around to). I
> think overall it is OK for next. I did find one or two nits, but I think
> they are things we can fix up in-tree if and when they become a problem
> (e.g., I noticed that test-genrandom gets piped to "perl -pe". I'm not
> sure if perl will complain about funny multibyte characters on some
> systems. I suggest we ignore it until somebody demonstrates that it
> actually matters).

I just looked, and that use is fine.  perl -pe is always going to treat
its data as bytes unless you use -C or explicitly enable Unicode
functionality.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* Re: [PATCH v12 3/8] graph: add support for --line-prefix on all graph-aware output
From: Dennis Kaarsemaker @ 2016-10-19 20:24 UTC (permalink / raw)
  To: Jacob Keller, git
  Cc: Junio C Hamano, Stefan Beller, Jeff King, Johannes Sixt,
	Jacob Keller
In-Reply-To: <20160831232725.28205-4-jacob.e.keller@intel.com>

On Wed, 2016-08-31 at 16:27 -0700, Jacob Keller wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
> 
> Add an extension to git-diff and git-log (and any other graph-aware
> displayable output) such that "--line-prefix=<string>" will print the
> additional line-prefix on every line of output.

This patch breaks git rev-list --header, also breaking gitweb.

The NUL between commits has gone missing, causing gitweb to interpret
the output of git rev-list as one commit.

Sorry for not catching this earlier, I actually encountered this early
september but thought it was caused by us running an ancient gitweb
with a modern git. Finally managed to upgrade gitweb today, and the bug
didn't go away. git bisect says 660e113ce is the culprit. Checking out
'next' and reverting this single patch makes the problem disappear.

Haven't yet tried to fix the bug, but this hunk looks suspicious:

-                       if (revs->commit_format != CMIT_FMT_USERFORMAT ||
-                           buf.len) {
-                               fwrite(buf.buf, 1, buf.len, stdout);
-                               putchar(info->hdr_termination);
-                       }
+                       /*
+                        * If the message buffer is empty, just show
+                        * the rest of the graph output for this
+                        * commit.
+                        */
+                       if (graph_show_remainder(revs->graph))
+                               putchar('\n');
+                       if (revs->commit_format == CMIT_FMT_ONELINE)
+                          

D.

^ permalink raw reply

* Re: Integrating submodules with no side effects
From: Robert Dailey @ 2016-10-19 19:51 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Git
In-Reply-To: <CAGZ79kZWaShG9iG565bD3aVYFcb53+opbpnJH-v3G6hrQL2tzA@mail.gmail.com>

On Wed, Oct 19, 2016 at 2:45 PM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Oct 19, 2016 at 12:19 PM, Robert Dailey
> <rcdailey.lists@gmail.com> wrote:
>> On Wed, Oct 19, 2016 at 11:23 AM, Stefan Beller <sbeller@google.com> wrote:
>>> You could try this patch series:
>>> https://github.com/jlehmann/git-submod-enhancements/tree/git-checkout-recurse-submodules
>>> (rebased to a newer version; no functional changes:)
>>> https://github.com/stefanbeller/git/tree/submodule-co
>>> (I'll rebase that later to origin/master)
>>>
>>>>
>>>> Do you have any info on how I can prevent that error? Ideally I want
>>>> the integration to go smoothly and transparently, not just for the
>>>> person doing the actual transition (me) but for everyone else that
>>>> gets those changes from upstream. They should not even notice that it
>>>> happened (i.e. no failed commands, awkward behavior, or manual steps).
>>>
>>> It depends on how long you want to postpone the transition, but I plan to
>>> upstream the series referenced above in the near future,
>>> which would enable your situation to Just Work (tm). ;)
>>
>> At first glance, what you've linked to essentially looks like
>> automated `git submodule update` for every `git checkout`. Am I
>> misunderstanding?
>
> Essentially yes, except with stricter rules than the actual submodule update
> IIRC.
>
>>
>> If I'm correct, this is not the same as what I'm talking about. The
>> problem appears to be more internal: When a submodule is removed, the
>> physical files that were there are not removed by Git.
>
> That is also done by that series: submodules ought to be treated as files:
> If you checkout a new version where a file is deleted, the checkout command
> will actually remove the file for you (and e.g. solve any
> directory/file conflicts
> that may happen in the transition.)
>
>> It leaves them
>> there in the working copy as untracked files.
>
> That is the current behavior as checkout tries hard to ignore submodules.
>
>> The next step Git takes
>> (again, just from outside observation) is to add those very same files
>> to the working copy, since they were added to a commit. However, at
>> this point Git fails because it's trying to create (write) files to
>> the working copy when an exact file of that name already exists there.
>> Git will not overwrite untracked files, so at this point it fails.
>>
>> What needs to happen, somehow, is Git sees that the files were
>> actually part of a submodule (which was removed) and remove the
>> physical files as well, assuming that they were not modified in the
>> submodule itself. This will ensure that the next step (creating the
>> files) will succeed since the files no longer block it.
>
> Yep.

It's great we're finally on the same page ;-)

However, I don't see how this problem can be solved with your script,
or solved in general outside of that. Does this mean that Git needs to
change to treat submodules as it does normal files, per your previous
assertion, which means submodules should *not* be left behind in the
working copy as untracked files?

^ permalink raw reply

* Re: Integrating submodules with no side effects
From: Stefan Beller @ 2016-10-19 19:45 UTC (permalink / raw)
  To: Robert Dailey; +Cc: Git
In-Reply-To: <CAHd499D_fehBHsNdH9PLQj+H_WAJqbbMhDwv_nCT3fZcX60ENA@mail.gmail.com>

On Wed, Oct 19, 2016 at 12:19 PM, Robert Dailey
<rcdailey.lists@gmail.com> wrote:
> On Wed, Oct 19, 2016 at 11:23 AM, Stefan Beller <sbeller@google.com> wrote:
>> You could try this patch series:
>> https://github.com/jlehmann/git-submod-enhancements/tree/git-checkout-recurse-submodules
>> (rebased to a newer version; no functional changes:)
>> https://github.com/stefanbeller/git/tree/submodule-co
>> (I'll rebase that later to origin/master)
>>
>>>
>>> Do you have any info on how I can prevent that error? Ideally I want
>>> the integration to go smoothly and transparently, not just for the
>>> person doing the actual transition (me) but for everyone else that
>>> gets those changes from upstream. They should not even notice that it
>>> happened (i.e. no failed commands, awkward behavior, or manual steps).
>>
>> It depends on how long you want to postpone the transition, but I plan to
>> upstream the series referenced above in the near future,
>> which would enable your situation to Just Work (tm). ;)
>
> At first glance, what you've linked to essentially looks like
> automated `git submodule update` for every `git checkout`. Am I
> misunderstanding?

Essentially yes, except with stricter rules than the actual submodule update
IIRC.

>
> If I'm correct, this is not the same as what I'm talking about. The
> problem appears to be more internal: When a submodule is removed, the
> physical files that were there are not removed by Git.

That is also done by that series: submodules ought to be treated as files:
If you checkout a new version where a file is deleted, the checkout command
will actually remove the file for you (and e.g. solve any
directory/file conflicts
that may happen in the transition.)

> It leaves them
> there in the working copy as untracked files.

That is the current behavior as checkout tries hard to ignore submodules.

> The next step Git takes
> (again, just from outside observation) is to add those very same files
> to the working copy, since they were added to a commit. However, at
> this point Git fails because it's trying to create (write) files to
> the working copy when an exact file of that name already exists there.
> Git will not overwrite untracked files, so at this point it fails.
>
> What needs to happen, somehow, is Git sees that the files were
> actually part of a submodule (which was removed) and remove the
> physical files as well, assuming that they were not modified in the
> submodule itself. This will ensure that the next step (creating the
> files) will succeed since the files no longer block it.

Yep.

^ permalink raw reply

* Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block
From: Junio C Hamano @ 2016-10-19 19:24 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: Stefan Beller, git@vger.kernel.org, Jakub Narębski
In-Reply-To: <31cae41d-701c-c657-f04e-3b617c0b1d39@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

> Would this work then:
> - at least one trailer line generated by Git ("(cherry picked by" or
>   "Signed-off-by: ") or configured in the "trailer" section in
>   git config AND at least 25% logical trailer lines
> OR
> - 100% logical trailer lines

Sure.  

At that point, I doubt that the latter "100% logical trailer" makes
much difference, though, because at least one of them is likely to
be in the former set, or the users can easily make it so by throwing
what they use like "Bug: ", "Test: " and "Change-ID: " in the
"configured in the trailer section" category.  FWIW, I do not think
we mind terribly to tweak the definition of "generated by Git" class
to "commonly used in Git managed projects" to include "Change-ID:"
and friends.

^ permalink raw reply

* Re: Integrating submodules with no side effects
From: Robert Dailey @ 2016-10-19 19:19 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Git
In-Reply-To: <CAGZ79kYKdHHcfRxCOJwNuqnOGMjBBeMy3Av5U7_ssuOp-to61w@mail.gmail.com>

On Wed, Oct 19, 2016 at 11:23 AM, Stefan Beller <sbeller@google.com> wrote:
> You could try this patch series:
> https://github.com/jlehmann/git-submod-enhancements/tree/git-checkout-recurse-submodules
> (rebased to a newer version; no functional changes:)
> https://github.com/stefanbeller/git/tree/submodule-co
> (I'll rebase that later to origin/master)
>
>>
>> Do you have any info on how I can prevent that error? Ideally I want
>> the integration to go smoothly and transparently, not just for the
>> person doing the actual transition (me) but for everyone else that
>> gets those changes from upstream. They should not even notice that it
>> happened (i.e. no failed commands, awkward behavior, or manual steps).
>
> It depends on how long you want to postpone the transition, but I plan to
> upstream the series referenced above in the near future,
> which would enable your situation to Just Work (tm). ;)

At first glance, what you've linked to essentially looks like
automated `git submodule update` for every `git checkout`. Am I
misunderstanding?

If I'm correct, this is not the same as what I'm talking about. The
problem appears to be more internal: When a submodule is removed, the
physical files that were there are not removed by Git. It leaves them
there in the working copy as untracked files. The next step Git takes
(again, just from outside observation) is to add those very same files
to the working copy, since they were added to a commit. However, at
this point Git fails because it's trying to create (write) files to
the working copy when an exact file of that name already exists there.
Git will not overwrite untracked files, so at this point it fails.

What needs to happen, somehow, is Git sees that the files were
actually part of a submodule (which was removed) and remove the
physical files as well, assuming that they were not modified in the
submodule itself. This will ensure that the next step (creating the
files) will succeed since the files no longer block it.

^ permalink raw reply

* Re: [PATCH v4 05/14] i18n: add--interactive: mark plural strings
From: Junio C Hamano @ 2016-10-19 18:40 UTC (permalink / raw)
  To: Vasco Almeida
  Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
	Jean-Noël AVILA, Jakub Narębski, David Aguilar
In-Reply-To: <20161010125449.7929-6-vascomalmeida@sapo.pt>

Vasco Almeida <vascomalmeida@sapo.pt> writes:

> @@ -669,12 +669,18 @@ sub status_cmd {
>  sub say_n_paths {
>  	my $did = shift @_;
>  	my $cnt = scalar @_;
> -	print "$did ";
> -	if (1 < $cnt) {
> -		print "$cnt paths\n";
> -	}
> -	else {
> -		print "one path\n";
> +	if ($did eq 'added') {
> +		printf(__n("added %d path\n", "added %d paths\n",
> +			   $cnt), $cnt);
> +	} elsif ($did eq 'updated') {
> +		printf(__n("updated %d path\n", "updated %d paths\n",
> +			   $cnt), $cnt);
> +	} elsif ($did eq 'reverted') {
> +		printf(__n("reverted %d path\n", "reverted %d paths\n",
> +			   $cnt), $cnt);
> +	} else {
> +		printf(__n("touched %d path\n", "touched %d paths\n",
> +			   $cnt), $cnt);
>  	}
>  }

Nice to see you covered all verbs currently in use and then
future-proofed by adding a fallback "touched" here.

Thanks.


^ permalink raw reply

* Re: [PATCH v4 01/14] i18n: add--interactive: mark strings for translation
From: Junio C Hamano @ 2016-10-19 18:14 UTC (permalink / raw)
  To: Vasco Almeida
  Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
	Jean-Noël AVILA, Jakub Narębski, David Aguilar
In-Reply-To: <20161010125449.7929-2-vascomalmeida@sapo.pt>

Vasco Almeida <vascomalmeida@sapo.pt> writes:

>  	} else {
> -		print "No untracked files.\n";
> +		print __("No untracked files.\n");
>  	}

Not a big deal, but this makes me wonder if we want to do this
instead

		print __("No untracked files.") . "\n";

so that translators do not have to remember to keep the final LF.

It seems there are some more hits of strings that end with "\n"
inside __(...) in this patch, so it probably is OK.

^ permalink raw reply

* Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block
From: Jonathan Tan @ 2016-10-19 18:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org, Jakub Narębski
In-Reply-To: <xmqqbmyhr4vt.fsf@gitster.mtv.corp.google.com>

On 10/18/2016 09:36 AM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>>>     * rs/c-auto-resets-attributes:
>>>       pretty: avoid adding reset for %C(auto) if output is empty
>>>
>>> And neither of the two colon containing line remotely resembles how
>>> a typical RFC-822 header is formatted.  So that may serve as a hint
>>> to how we can tighten it without introducing false negative.
>>
>> The only "offending" character is the space (according to RFC 822),
>> but that sounds like a good rule to have.
>
> I suspect that we should be willing to deviate from the letter of
> RFC to reject misidentification.  I saw things like
>
> 	Thanks to: Jonathan Tan <jt@host.xz>
> 	Signed-off-by: A U Thor <au@th.or>
>
> in the wild (notice the SP between Thanks and to), for example.
> Rejecting leading whitespace as a line that does *not* start the
> header (hence its colon does not count) may be a good compromise.

Good point.

>> I think that "Signed-off-by:" is not guaranteed to be
>> present.
>
> But do we really care about that case where there is no S-o-b:?  I
> personally do not think so.

I think we should - the use case I had in mind when I started this is 
the Android Git repository, which does not use Signed-off-by. For 
example, I quoted this commit in an earlier e-mail [1]:

https://android.googlesource.com/platform/frameworks/base/+/4c5281862f750cbc9d7355a07ef1a5545b9b3523

which has the footer:

   Bug: http://b/30562229
   Test: readelf --dyn-sym app_process32 and check that bsd_signal is 
exported
         readelf --dyn-sym app_process64 and check that bsd_signal is 
not exported
   Change-Id: Iec584070b42bc7fa43b114c0f884aff2db5a6858

>> Defining a trailer line as "a line starting with a token,
>> then optional whitespace, then separator", maybe the following rule:
>> - at least one trailer line generated by Git ("(cherry picked by" or
>> "Signed-off-by") or configured in the "trailer" section in gitconfig
>> OR
>> - at least 3/4 logical trailer lines (I'm wondering if this should be
>> 100% trailer lines)
>
> I'd strongly suggest turning that OR to AND.  We will not safely be
> able to write a commit log message that describes how S-o-b lines
> are handled in its last paragraph otherwise.
>
> I do not care too deeply about 3/4, but I meant to allow 75% cruft
> but no more than that, and the fact that the threashold is set at
> way more than 50% is important.  IOW, if you have
>
> 	Ordinary log message here...
>
> 	S-o-b: A U Thor <au@th.or>
> 	[a short description that is typically a single liner
>         in the real world use pattern we saw in the world, but
> 	could overflow to become multi line cruft]
> 	S-o-b: R E Layer <re@lay.er>
>
> "last paragraph" is 5 lines long, among which 60% are cruft that is
> below the 75% threshold, and "am -s" can still add the S-o-b of the
> committer at the end of that existing last paragraph.  Making it too
> strict would raise the false negative ratio.

Ah, sorry, I misread your original suggestion.

Would this work then:
- at least one trailer line generated by Git ("(cherry picked by" or
   "Signed-off-by: ") or configured in the "trailer" section in
   git config AND at least 25% logical trailer lines
OR
- 100% logical trailer lines

The first part is your original suggestion except that I think that it 
is more consistent to allow other trailer lines as well (but I do not 
feel too strongly about this). The second part would satisfy the Android 
Git use case above, and also retain existing behavior when 
"Signed-off-by" (for example) is added to an existing footer that does 
not contain "Signed-off-by" yet.

What do you think?

[1] Message ID <29cb0f55-f729-80af-cdca-64e927fa97c0@google.com>

^ permalink raw reply

* Re: [PATCH 5/7] merge-base: mark bases that are on first-parent chain
From: Junio C Hamano @ 2016-10-19 17:42 UTC (permalink / raw)
  To: git
In-Reply-To: <20161019042345.29766-6-gitster@pobox.com>

Junio C Hamano <gitster@pobox.com> writes:

> In a workflow where topic branches are first merged to the 'next'
> integration branch to be tested before getting merged down to the
> 'master' integration branch to be consumed by the end users, merging
> the 'master' branch back to the 'next' happens after topics graduate
> to 'master' and release notes entries are written for them.
>
> Git finds many merge bases between 'master' and 'next' while
> creating this merge.  In addition to the tip of 'master' back when
> we made such a merge back from 'master' to 'next' was made the last
> time, which is the most reasonable merge base to explain the
> histories of both branches, all the tips of topic branches that
> graduated recently are merge bases.  Because these tips of topic
> branches were already in 'next', the tip of 'next' reaches them, and
> because they just graduated to 'master', the tip of 'master' reaches
> them, too.  And these topics are independent most of the time (that
> is the point of employing the topic-branch workflow), so they cannot
> be reduced.

The idea here is to exclude tips of topic branches as "obviously
less useful as merge bases than the mainline commit".  Aside from
the fact that the approach is purely a heuristic that heavily relies
on convention aka "topic branch workflow", there is a caveat (or
more that I haven't thought of yet).  

One is that there may be no merge base found that is on the first
parent chain with this particular patch.  When any of the topics
that just graduated to 'master' was forked off of 'master' after the
latest merge from 'master' to 'next' was made, then the merge base
on the first parent chain, i.e. the commit on 'master' that was
merged to 'next' the last time, will be an ancestor of the tip of
that recently forked topic, which is another common ancestor between
'master' and 'next' being merged (hence removed as redundant).  We
will be left with only the merge bases that are off first-parent
chain, and I think "git merge" will say "no related histories; I
won't merge them".

I do not know if that is a problem in practice, but if it turns out
to be, it probably can be corrected by updating the way how the
paint_down_to_common() function works.  It still stops traversal,
even with this patch, when a commit is found to be common and place
it to the "potential merge bases" list, but instead we could keep
digging until we hit a commit that is common between PARENT1 and
PARENT2 and also is on the first-parent chain, without declaring
that we found one result.

But that probably ends up being the same as ignoring all side
branches from merges and finding merge bases only paying attention
to the first-parent chain.

^ permalink raw reply

* Re: Drastic jump in the time required for the test suite
From: Junio C Hamano @ 2016-10-19 17:32 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1610191049040.3847@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> What I can also say for certain is that the version from yesterday (commit
> 4ef44ce) was the first one in a week that built successfully and hence
> reached the test phase, and it was the first version of `pu` ever to time
> out after running for 3h.

I am sympathetic, but I'd be lying if I said I can feel your pain.

Admittedly I do not run _all_ the tests (I certainly know that I
exclude the ones behind EXPENSIVE prerequisite), but after every
rebuilding of 'jch' and 'pu', I run the testsuite (and also rebuild
docs) before pushing them out, and "make test && make doc && make
install install-doc" run sequentially for the four integration
branches finishes within 15 minutes, even when I run them after
"make clean".

Perhaps the recent change to run the tests in parallel from slower
to faster under prove may be helping my case.

> Maybe we should start optimizing the tests...

Yup, two things that come to mind are to identify long ones and see
if each of them can be split into two halves that can be run in
parallel, and to go through the tests with fine toothed comb and
remove the ones that test exactly the same thing as another test.
The latter would be very time consuming, though.

^ permalink raw reply

* Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item
From: Santiago Torres @ 2016-10-19 17:07 UTC (permalink / raw)
  To: Jeff King; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161019091641.vcv3snlg5xr3yazs@sigill.intra.peff.net>

[-- Attachment #1: Type: text/plain, Size: 1623 bytes --]

On Wed, Oct 19, 2016 at 05:16:42AM -0400, Jeff King wrote:
> On Wed, Oct 19, 2016 at 04:55:43AM -0400, Jeff King wrote:
> 
> > > diff --git a/ref-filter.h b/ref-filter.h
> > > index 14d435e..3d23090 100644
> > > --- a/ref-filter.h
> > > +++ b/ref-filter.h
> > > @@ -107,4 +107,7 @@ struct ref_sorting *ref_default_sorting(void);
> > >  /*  Function to parse --merged and --no-merged options */
> > >  int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
> > >  
> > > +void pretty_print_ref(const char *name, const unsigned char *sha1,
> > > +		const char *format, unsigned kind);
> > > +
> > 
> > What are the possible values for "kind"? I guess these should come from
> > FILTER_REFS_TAGS, BRANCHES, etc. It's probably worth documenting that.
> > Alternatively, is it possible to just determine this from the name? It
> > looks like filter_ref_kind() is how it happens for a normal ref-filter.
> 
> I guess that may complicate things for the caller you add in this
> series, which may not have a fully-qualified refname (which is obviously
> how filter_ref_kind() figures it out). I'd argue that is a bug, though,
> as things like "%(refname)" are generally expected to print out the
> fully refname ("git tag --format=%(refname)" does so, and you can use
> "%(refname:short)" if you want the shorter part).

Hmm, I hadn't actually noticed that. Do you have any suggestions in how to
address this?

In general this feels like a consequence of disambiguating .git/tags/*
within builtin/tag.c rather than letting ref-filter figure it out.

Thanks,
-Santiago.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply

* Re: [PATCH] sha1_name: remove ONELINE_SEEN bit
From: Johannes Schindelin @ 2016-10-19  7:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqd1ixphog.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Tue, 18 Oct 2016, Junio C Hamano wrote:

> 28a4d94044 ("object name: introduce ':/<oneline prefix>' notation",
> 2007-02-24) started using its own bit from object->flags to mark
> commits used while parsing the ":/token" extended SHA-1 syntax to
> name a commit temporarily, and this was kept even when f7bff00314
> ("sha1_name.c: fix parsing of ":/token" syntax", 2010-08-02) found
> and fixed a bug in its implementation.
> 
> The use of that flag bit, however, is limited to a single function,
> get_sha1_oneline(), which first sets it for the commits sitting at
> the tips of refs, uses the bit to avoid duplicate traversal while
> walking the history, and then cleans the bit from all commits it
> walked.
> 
> Which is exactly what the general-purpose TMP_MARK bit meant to be
> used for isolated case was invented for.  Replace ONELINE_SEEN with
> TMP_MARK and retire the former.

ACK,
Dscho

^ permalink raw reply

* Re: Integrating submodules with no side effects
From: Stefan Beller @ 2016-10-19 16:23 UTC (permalink / raw)
  To: Robert Dailey; +Cc: Git
In-Reply-To: <CAHd499CN3+cHVwjOEirwXVu3DsJwPrmJwEgSJL2CHD5CvoYxxg@mail.gmail.com>

On Wed, Oct 19, 2016 at 6:27 AM, Robert Dailey <rcdailey.lists@gmail.com> wrote:
> On Tue, Oct 18, 2016 at 4:17 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Tue, Oct 18, 2016 at 12:35 PM, Robert Dailey
>> <rcdailey.lists@gmail.com> wrote:
>>> Hello git experts,
>>>
>>> I have in the past attempted to integrate submodules into my primary
>>> repository using the same directory name. However, this has always
>>> caused headache when going to and from branches that take you between
>>> when this integration occurred and when it didn't. It's a bit hard to
>>> explain. Basically, if I have a submodule "foo", and I delete that
>>> submodule and physically add its files under the same directory "foo",
>>> when I do a pull to get this change from another clone, it fails
>>> saying:
>>>
>>> error: The following untracked working tree files would be overwritten
>>> by checkout:
>>>         foo/somefile.txt
>>> Please move or remove them before you switch branches.
>>> Aborting
>>> could not detach HEAD
>>>
>>>
>>> Obviously, git can't delete the submodule because the files have also
>>> been added directly. I don't think it is built to handle this
>>> scenario. Here is the series of commands I ran to "integrate" the
>>> submodule (replace the submodule with a directory containing the exact
>>> contents of the submodule itself):
>>>
>>> #!/usr/bin/env bash
>>> mv "$1" "${1}_"
>>> git submodule deinit "$1"
>>
>> This removes the submodule entries from .git/config
>> (and it would remove the contents of that submodule, but they are moved)
>>
>>> git rm "$1"
>>
>> Removing the git link here.
>>
>> So we still have the entries in the .gitmodules file there.
>> Maybe add:
>>
>>     name=$(git submodule-helper name $1)
>>     git config -f .gitmodules --unset submodule.$name.*
>>     git add .gitmodules
>>
>> ? (Could be optional)
>
> Actually I verified that it seems `git rm` is specialized for
> submodules somewhere, because when I run that command on a submodule
> the relevant entries in the .gitmodules file are removed. I did not
> have to do this as a separate step.
>
>>> mv "${1}_" "$1"
>>> git add "$1/**"
>>
>> Moving back into place and adding all files in there.
>>
>>>
>>> The above script is named git-integrate-submodule, I run it like so:
>>>
>>> $ git integrate-submodule foo
>>>
>>> Then I do:
>>>
>>> $ git commit -m 'Integrated foo submodule'
>>>
>>> Is there any way to make this work nicely?
>>
>> I think you can just remove the gitlink from the index and not from the working
>> tree ("git rm --cached $1")
>
> What is the goal of doing it this way? What does this simplify?

You don't have to mv it back and forth with an underscore I would imagine?

>
>>> The only solution I've
>>> found is to obviously rename the directory before adding the physical
>>> files, for example name it foo1. Because they're different, they never
>>> "clash".
>>
>> Also look at the difference between plumbing and porcelain commands[1],
>> as plumbing is more stable than the porcelain, so it will be easier to maintain
>> this script.
>
> Which plumbing commands did you have in mind?

None specifically. I write scripts using porcelain all the time for
personal use.
But if you were planning to publish this seriously then I'd recommend looking at
plumbing commands.

>
>> I think this would be an actually reasonable feature, which Git itself
>> could support via "git submodule [de]integrate", but then we'd also want
>> to see the reverse, i.e. take a sub directory and make it a submodule.
>
> Integrating this as a feature might be fine, I think when you bring up
> the question of retaining history makes things much harder.
> Fortunately for me that is not a requirement in this case, so I'm able
> to do things with much less effort.

That reminds me of subtree merging, which could be used for this case.
(see 'git subtree')

>
> However the primary purpose of my post was to find out how to
> integrate the submodule without the error on next pull by other
> collaborators of my repository. It's a real pain to recover your
> working copy when going inbetween commits where the submodule
> integration happened inbetween them. I did quote the exact error
> message I got in my original post.

You could try this patch series:
https://github.com/jlehmann/git-submod-enhancements/tree/git-checkout-recurse-submodules
(rebased to a newer version; no functional changes:)
https://github.com/stefanbeller/git/tree/submodule-co
(I'll rebase that later to origin/master)

>
> Do you have any info on how I can prevent that error? Ideally I want
> the integration to go smoothly and transparently, not just for the
> person doing the actual transition (me) but for everyone else that
> gets those changes from upstream. They should not even notice that it
> happened (i.e. no failed commands, awkward behavior, or manual steps).

It depends on how long you want to postpone the transition, but I plan to
upstream the series referenced above in the near future,
which would enable your situation to Just Work (tm). ;)

^ permalink raw reply

* [PATCH] Add a knob to abort on die() (was Re: git checkout crashes after ...)
From: Duy Nguyen @ 2016-10-19 13:47 UTC (permalink / raw)
  To: git; +Cc: Raffael Reichelt
In-Reply-To: <CACsJy8B50daiHWfu7zfVQnn_i_=HbUK3gBPmv1U=EUw7ZyuGPw@mail.gmail.com>

On Wed, Oct 19, 2016 at 08:27:43PM +0700, Duy Nguyen wrote:
> If you set the environment variable GIT_ALLOC_LIMIT ...  git
> attempts to allocate more than that ... then it's caught and we get
> a glimpse of how much memory git may need. Unfortunately we can't
> get a stack trace or anything like that unless you rebuild Git from
> source.

It's moments like this that I wish we had a knob to force core
dumps. And I often modify die_builtin() to add '*(char*)0 = 1;' to
force a core dump when I can't figure out some problem based on the
error message alone.

So.. how about we do something like this? We could extend it to abort
on error() as well as die(). Aborting on warning() may be a bit too
much though. On glibc systems we could even print the back trace
without aborting, which helps in some cases.

The long variable name and value are on purpose to hopefully not
trigger this by mistake.

diff --git a/git.c b/git.c
index ab5c99c..5fea224 100644
--- a/git.c
+++ b/git.c
@@ -622,15 +622,34 @@ static int run_argv(int *argcp, const char ***argv)
 	return done_alias;
 }
 
+static NORETURN void die_by_aborting(const char *err, va_list params)
+{
+	vreportf("fatal: ", err, params);
+	abort();
+}
+
+static NORETURN void die_silently_by_aborting(const char *err, va_list params)
+{
+	abort();
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	const char *cmd;
 	int done_help = 0;
+	const char *die_abort_env = getenv("GIT_ABORT_ON_FATAL_ERRORS");
 
 	cmd = argv[0];
 	if (!cmd)
 		cmd = "git-help";
 
+	if (die_abort_env) {
+		if (!strcmp(die_abort_env, "yes please"))
+			set_die_routine(die_by_aborting);
+		else if (!strcmp(die_abort_env, "just die"))
+			set_die_routine(die_silently_by_aborting);
+	}
+
 	trace_command_performance(argv);
 
 	/*
--
Duy

^ permalink raw reply related

* Re: [PATCH v4 1/7] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: Jeff King @ 2016-10-19  8:51 UTC (permalink / raw)
  To: santiago; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161007210721.20437-2-santiago@nyu.edu>

On Fri, Oct 07, 2016 at 05:07:15PM -0400, santiago@nyu.edu wrote:

> From: Lukas Puehringer <luk.puehringer@gmail.com>
> 
> Functions that print git object information may require that the
> gpg-interface functions be silent. Add GPG_VERIFY_QUIET flag and prevent
> print_signature_buffer from being called if flag is set.

The layering here is a little funny. The gpg-interface code allocates a
new flag, but none of its functions do anything with it.  Instead, it's
acted on only by the run_gpg_verify() command local to tag.c.

I guess this "flags" variable comes to us from other callsites via
gpg_verify_tag. That's still in tag.c, but it's arguably part of the gpg
interface.

So I think it's OK.

-Peff

^ permalink raw reply

* Drastic jump in the time required for the test suite
From: Johannes Schindelin @ 2016-10-19  9:18 UTC (permalink / raw)
  To: gitster; +Cc: git

Hi Junio,

I know you are a fan of testing things thoroughly in the test suite, but I
have to say that it is getting out of hand, in particular due to our
over-use of shell script idioms (which really only run fast on Linux, not
a good idea for a portable software).

My builds of `pu` now time out, after running for 3h straight in the VM
dedicated to perform the daily routine of building and testing the git.git
branches in Git for Windows' SDK. For comparison, `next` passes build &
tests in 2.6h. That is quite the jump.

Unfortunately, I cannot tell you precisely when this massive increase
happened (we are talking about half an hour, after all), because there
were build failures preventing the test from being run for the last 40
days (and my build job only retains the results for 7 days currently,
anyway, in addition to the last successful/unsuccessful build).

What I can say for certain is that the version from 41 days ago (commit
7837d4a) ran for only 2.6h, just as `next` does today, and passed without
failure.

Every single revision of `pu` since then has been broken in one way or
another. Mostly already the build, so that the tests would not even start
to run.

What I can also say for certain is that the version from yesterday (commit
4ef44ce) was the first one in a week that built successfully and hence
reached the test phase, and it was the first version of `pu` ever to time
out after running for 3h.

I will increase the time-out, of course, but we are walking into a
dangerous territory here: the build & test requires such an incredibly
long time now that the VM will start to take more than 24h to complete all
8 tasks (building & testing the `pu`, `next`, `master` and `maint`
branches, as well as trying to rebase Git for Windows' patches onto each
of them).

As those tasks are scheduled to run once every 24h, I will no longer be
able to notify you and the mailing list in a timely manner, if at all,
because the queue will clog up.

Maybe we should start optimizing the tests to become more useful again, by
forcing them not to take such an insane amount of time?

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item
From: Jeff King @ 2016-10-19  8:55 UTC (permalink / raw)
  To: santiago; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161007210721.20437-3-santiago@nyu.edu>

On Fri, Oct 07, 2016 at 05:07:16PM -0400, santiago@nyu.edu wrote:

> From: Lukas Puehringer <luk.puehringer@gmail.com>
> 
> ref-filter functions are useful for printing git object information
> using a format specifier. However, some other modules may not want to use
> this functionality on a ref-array but only print a single item.
> 
> Expose a pretty_print_ref function to create, pretty print and free
> individual ref-items.

Makes sense.

> diff --git a/ref-filter.h b/ref-filter.h
> index 14d435e..3d23090 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -107,4 +107,7 @@ struct ref_sorting *ref_default_sorting(void);
>  /*  Function to parse --merged and --no-merged options */
>  int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
>  
> +void pretty_print_ref(const char *name, const unsigned char *sha1,
> +		const char *format, unsigned kind);
> +

What are the possible values for "kind"? I guess these should come from
FILTER_REFS_TAGS, BRANCHES, etc. It's probably worth documenting that.
Alternatively, is it possible to just determine this from the name? It
looks like filter_ref_kind() is how it happens for a normal ref-filter.

-Peff

^ permalink raw reply

* Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item
From: Jeff King @ 2016-10-19  9:16 UTC (permalink / raw)
  To: santiago; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161019085543.om7v7eowfaushags@sigill.intra.peff.net>

On Wed, Oct 19, 2016 at 04:55:43AM -0400, Jeff King wrote:

> > diff --git a/ref-filter.h b/ref-filter.h
> > index 14d435e..3d23090 100644
> > --- a/ref-filter.h
> > +++ b/ref-filter.h
> > @@ -107,4 +107,7 @@ struct ref_sorting *ref_default_sorting(void);
> >  /*  Function to parse --merged and --no-merged options */
> >  int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
> >  
> > +void pretty_print_ref(const char *name, const unsigned char *sha1,
> > +		const char *format, unsigned kind);
> > +
> 
> What are the possible values for "kind"? I guess these should come from
> FILTER_REFS_TAGS, BRANCHES, etc. It's probably worth documenting that.
> Alternatively, is it possible to just determine this from the name? It
> looks like filter_ref_kind() is how it happens for a normal ref-filter.

I guess that may complicate things for the caller you add in this
series, which may not have a fully-qualified refname (which is obviously
how filter_ref_kind() figures it out). I'd argue that is a bug, though,
as things like "%(refname)" are generally expected to print out the
fully refname ("git tag --format=%(refname)" does so, and you can use
"%(refname:short)" if you want the shorter part).

-Peff

^ permalink raw reply

* Re: [PATCH v4 5/7] builtin/tag: add --format argument for tag -v
From: Jeff King @ 2016-10-19  9:10 UTC (permalink / raw)
  To: santiago; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161007210721.20437-6-santiago@nyu.edu>

On Fri, Oct 07, 2016 at 05:07:19PM -0400, santiago@nyu.edu wrote:

> Adding --format to git tag -v mutes the default output of the GPG
> verification and instead prints the formatted tag object.

The same comments apply to "mutes" here, as to the previous patch (which
is to say I think we may want something more, but this is probably OK
for now).

> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 7ecca8e..3bb5e3c 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -15,7 +15,7 @@ SYNOPSIS
>  'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
>  	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
>  	[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
> -'git tag' -v <tagname>...
> +'git tag' -v [--format=<format>] <tagname>...

Just thinking out loud, but if we had ref-filter placeholders that
triggered GPG verification, you could do this all with the listing mode,
like:

  git tag --format='%(gpgstatus) %(tag) %(refname:short)'

and verify multiple tags (or give a single tag to limit it to just one).

I don't think that's any kind of blocker for this series. We already
have "-v", and adding --format to it is reasonable, even if we
eventually move to a world where people can use the listing mode. Like I
said, just thinking out loud.

> +static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
> +		void *cb_data)
>  {
>  	const char **p;
>  	char ref[PATH_MAX];
>  	int had_error = 0;
>  	unsigned char sha1[20];
>  
> +
>  	for (p = argv; *p; p++) {

Extra space?

>  static int verify_tag(const char *name, const char *ref,
> -				const unsigned char *sha1)
> +				const unsigned char *sha1, void *cb_data)
>  {
> -	return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> +	int flags;

Probably doesn't matter much, but these flags are defined as "unsigned"
elsewhere.

-Peff

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox