* Re: [BUG] fetch output is ugly in 'next'
From: Jeff King @ 2016-10-21 21:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <xmqqd1itd4t2.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 21, 2016 at 09:50:49AM -0700, Junio C Hamano wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > On Fri, Oct 21, 2016 at 7:11 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> >> Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something
> >> sensible should do it.
> >
> > Correction (if somebody will pick this up), it's
> > TRANSPORT_SUMMARY_WIDTH that needs to be adjusted, not those four.
>
> Yes, it used to be and it still is (2 * DEFAULT_ABBREV + 3) but in
> the new world order where default-abbrev is often -1 the expression
> does not make much sense.
>
> Perhaps something along this line?
>
> transport.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/transport.h b/transport.h
> index 18d2cf8275..5339fabbad 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -127,7 +127,7 @@ struct transport {
> #define TRANSPORT_PUSH_CERT 2048
> #define TRANSPORT_PUSH_ATOMIC 4096
>
> -#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
> +#define TRANSPORT_SUMMARY_WIDTH (2 * (DEFAULT_ABBREV < 0 ? 7 : DEFAULT_ABBREV) + 3)
> #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
That doesn't apply on 'next', because we have already done the
equivalent there. :)
The right way to spell "7" is FALLBACK_DEFAULT_ABBREV, which is handled
by your 65acfeacaa.
The remaining issue is that the static abbreviation is not nearly long
enough for git.git anymore; the auto-abbrev feature bumps my repo to a
minimum of 10 characters (it may only be 9 on a fresh clone; I have a
couple remotes and active work in progress). So this isn't exactly a
regression; it has always been the case that we may mis-align when the
abbreviations ended up longer than the minimum. It's just that it didn't
happen all that often in most repos (but it probably did constantly in
linux.git).
The simplest band-aid fix would be to compute TRANSPORT_SUMMARY_WIDTH on
the fly, taking into account the minimum found by actually counting the
objects. That at least gets us back to where we were, with it mostly
working and occasionally ugly when there's an oddball collision (for
git.git anyway; it probably makes the kernel output much nicer).
The "right" fix is to queue up the list of ref updates to print, find
the abbreviations for each, and then print them all in one shot, knowing
ahead of time the size necessary to align them. This could also let us
improve the name-alignment.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] test-lib: add --verbose-log option
From: Jeff King @ 2016-10-21 21:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, Lars Schneider, git
In-Reply-To: <xmqq7f91d3tb.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 21, 2016 at 10:12:16AM -0700, Junio C Hamano wrote:
> > -if test "$verbose" = "t"
> > +if test "$verbose_log" = "t"
> > +then
> > + exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
> > +elif test "$verbose" = "t"
> > then
> > exec 4>&2 3>&1
> > else
>
> OK, unlike "verbose" case where we give 3 (saved stdout) to 1 and 4
> (saved stderr) to 2, we send 3 to the output file and 4 to the same.
Your comment made me double-check whether we ought to be separating the
two in any way. Because my statement earlier in the thread that the
--verbose output goes to stdout is not _entirely_ true. It goes to
stdout and stderr, as written by the various programs in the tests.
So:
./t0000-whatever.sh -v | less
isn't quite sufficient. You really do want "2>&1" in there to catch
everything.
But for the case of "--tee", we already do so, in order to make sure it
all goes to the logfile. And since this option always implies "--tee",
it is correct here to send "3" and "4" to the same place.
So the patch is correct. But when I said earlier that people might be
annoyed if we just sent --verbose output to stderr, that probably isn't
true. We could perhaps make:
prove t0000-whatever.sh :: -v
"just work" by sending all of the verbose output to stderr. I don't
think it really matters, though. Nobody is doing that, because it's
pointless without "--tee".
-Peff
^ permalink raw reply
* Re: generating combined diff without an existing merge commit
From: Jacob Keller @ 2016-10-21 22:01 UTC (permalink / raw)
To: Git mailing list
In-Reply-To: <CA+P7+xrN-_zP40uAUGtqZW+OO4D4Z65SiPRykdKvauO1zgNNcQ@mail.gmail.com>
On Fri, Oct 21, 2016 at 2:40 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> Hi,
>
> I recently determined that I can produce an interdiff for a series
> that handles rebasing nicely and shows the conflicts resolved when
> rebasing plus any other changes.
>
> The basic idea is something like the following, assuming that v1 is a
> tag that points to the first version, v2 is a tag that points to the
> rebased new version, and base is a tag that points to the new base of
> the series (ie: the upstream if the v2 is on a branch and has been
> fully rebased)
>
> git checkout v1
> git merge base
> #perform any further edits to get everything looking like v2
> git commit
> git show -cc HEAD
>
> This is also equivalent to the following without having to actually do
> the merge manually:
>
> git commit-tree v2^{head} -p v1 -p master -m "some merge message"
> git show <output from the commit tree above)
>
> this nicely shows us the combined diff format which correctly shows
> any conflicts required to fix up during the rebase (which we already
> did because we have v2) and it also shows any *other* changes caused
> by v2 but without showing changes which we didn't actually make. (I
> think?)
>
> The result is that we can nicely see what was required to produce v2
> from v1 but without being cluttered by what changed in base.
>
> However, I have to actually generate the commit to do this. I am
> wondering if it is possible today to actually just do something like:
>
> git diff <treeish> <treeish> <treeish> and get the result that I want?
>
> I've already started digging to see if I can do that but haven't found
> anything yet.
>
> Thanks,
> Jake
Turns out that somehow I must have messed up my command because "git
diff <treeish> <treeish> <treeish>" does indeed do exactly what I
want.
Thanks,
Jake
^ permalink raw reply
* Re: [BUG] fetch output is ugly in 'next'
From: Junio C Hamano @ 2016-10-21 22:14 UTC (permalink / raw)
To: Jeff King; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <20161021214218.u46qf3lch3wwiutp@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The "right" fix is to queue up the list of ref updates to print, find
> the abbreviations for each, and then print them all in one shot, knowing
> ahead of time the size necessary to align them. This could also let us
> improve the name-alignment.
Heh, that was what I was starting to code while listening to
Jonathan and Stefan talk in a meeting ;-)
^ permalink raw reply
* [PATCH 2/3] fetch: pass summary_width down the callchain
From: Junio C Hamano @ 2016-10-21 22:39 UTC (permalink / raw)
To: git
In-Reply-To: <20161021223927.26364-1-gitster@pobox.com>
The leaf function on the "fetch" side that uses TRANSPORT_SUMMARY_WIDTH
constant is builtin/fetch.c::format_display() and it has two distinct
callchains. The one that reports the primary result of fetch originates
at store_updated_refs(); the other one that reports the pruning of
the remote-tracking refs originates at prune_refs().
Teach these two places to pass summary_width down the callchain,
just like we did for the "push" side in the previous commit.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/fetch.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a9f12cc5cf..40696e5338 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -17,9 +17,6 @@
#include "argv-array.h"
#include "utf8.h"
-#define TRANSPORT_SUMMARY(x) \
- (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
-
static const char * const builtin_fetch_usage[] = {
N_("git fetch [<options>] [<repository> [<refspec>...]]"),
N_("git fetch [<options>] <group>"),
@@ -569,9 +566,12 @@ static void print_compact(struct strbuf *display,
static void format_display(struct strbuf *display, char code,
const char *summary, const char *error,
- const char *remote, const char *local)
+ const char *remote, const char *local,
+ int summary_width)
{
- strbuf_addf(display, "%c %-*s ", code, TRANSPORT_SUMMARY(summary));
+ int width = (summary_width + strlen(summary) - gettext_width(summary));
+
+ strbuf_addf(display, "%c %-*s ", code, width, summary);
if (!compact_format)
print_remote_to_local(display, remote, local);
else
@@ -583,7 +583,8 @@ static void format_display(struct strbuf *display, char code,
static int update_local_ref(struct ref *ref,
const char *remote,
const struct ref *remote_ref,
- struct strbuf *display)
+ struct strbuf *display,
+ int summary_width)
{
struct commit *current = NULL, *updated;
enum object_type type;
@@ -597,7 +598,7 @@ static int update_local_ref(struct ref *ref,
if (!oidcmp(&ref->old_oid, &ref->new_oid)) {
if (verbosity > 0)
format_display(display, '=', _("[up to date]"), NULL,
- remote, pretty_ref);
+ remote, pretty_ref, summary_width);
return 0;
}
@@ -611,7 +612,7 @@ static int update_local_ref(struct ref *ref,
*/
format_display(display, '!', _("[rejected]"),
_("can't fetch in current branch"),
- remote, pretty_ref);
+ remote, pretty_ref, summary_width);
return 1;
}
@@ -621,7 +622,7 @@ static int update_local_ref(struct ref *ref,
r = s_update_ref("updating tag", ref, 0);
format_display(display, r ? '!' : 't', _("[tag update]"),
r ? _("unable to update local ref") : NULL,
- remote, pretty_ref);
+ remote, pretty_ref, summary_width);
return r;
}
@@ -654,7 +655,7 @@ static int update_local_ref(struct ref *ref,
r = s_update_ref(msg, ref, 0);
format_display(display, r ? '!' : '*', what,
r ? _("unable to update local ref") : NULL,
- remote, pretty_ref);
+ remote, pretty_ref, summary_width);
return r;
}
@@ -670,7 +671,7 @@ static int update_local_ref(struct ref *ref,
r = s_update_ref("fast-forward", ref, 1);
format_display(display, r ? '!' : ' ', quickref.buf,
r ? _("unable to update local ref") : NULL,
- remote, pretty_ref);
+ remote, pretty_ref, summary_width);
strbuf_release(&quickref);
return r;
} else if (force || ref->force) {
@@ -685,12 +686,12 @@ static int update_local_ref(struct ref *ref,
r = s_update_ref("forced-update", ref, 1);
format_display(display, r ? '!' : '+', quickref.buf,
r ? _("unable to update local ref") : _("forced update"),
- remote, pretty_ref);
+ remote, pretty_ref, summary_width);
strbuf_release(&quickref);
return r;
} else {
format_display(display, '!', _("[rejected]"), _("non-fast-forward"),
- remote, pretty_ref);
+ remote, pretty_ref, summary_width);
return 1;
}
}
@@ -721,6 +722,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
char *url;
const char *filename = dry_run ? "/dev/null" : git_path_fetch_head();
int want_status;
+ int summary_width = TRANSPORT_SUMMARY_WIDTH;
fp = fopen(filename, "a");
if (!fp)
@@ -830,13 +832,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
strbuf_reset(¬e);
if (ref) {
- rc |= update_local_ref(ref, what, rm, ¬e);
+ rc |= update_local_ref(ref, what, rm, ¬e,
+ summary_width);
free(ref);
} else
format_display(¬e, '*',
*kind ? kind : "branch", NULL,
*what ? what : "HEAD",
- "FETCH_HEAD");
+ "FETCH_HEAD", summary_width);
if (note.len) {
if (verbosity >= 0 && !shown_url) {
fprintf(stderr, _("From %.*s\n"),
@@ -903,6 +906,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
int url_len, i, result = 0;
struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, ref_map);
char *url;
+ int summary_width = TRANSPORT_SUMMARY_WIDTH;
const char *dangling_msg = dry_run
? _(" (%s will become dangling)")
: _(" (%s has become dangling)");
@@ -938,7 +942,8 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
shown_url = 1;
}
format_display(&sb, '-', _("[deleted]"), NULL,
- _("(none)"), prettify_refname(ref->name));
+ _("(none)"), prettify_refname(ref->name),
+ summary_width);
fprintf(stderr, " %s\n",sb.buf);
strbuf_release(&sb);
warn_dangling_symref(stderr, dangling_msg, ref->name);
--
2.10.1-723-g2384e83bc3
^ permalink raw reply related
* [PATCH 0/3] fetch output is ugly in 'next'
From: Junio C Hamano @ 2016-10-21 22:39 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqa8dxbb9r.fsf@gitster.mtv.corp.google.com>
It turns out that there are three codepaths, all of which have a set
of refs that they want to show them using TRANSPORT_SUMMARY_WIDTH
constant. The two preparatory steps turn the constant used at the
leaf level into a parameter that is passed down through these
callchains, and the last step introduces a helper function that can
be used to compute the appropriate width to be fed to these
callchains.
Junio C Hamano (3):
transport: pass summary_width down the callchain
fetch: pass summary_width down the callchain
transport: allow summary-width to be computed dynamically
builtin/fetch.c | 37 +++++++++++++++++--------------
transport.c | 68 ++++++++++++++++++++++++++++++++++++---------------------
transport.h | 2 +-
3 files changed, 65 insertions(+), 42 deletions(-)
--
2.10.1-723-g2384e83bc3
^ permalink raw reply
* [PATCH 1/3] transport: pass summary_width down the callchain
From: Junio C Hamano @ 2016-10-21 22:39 UTC (permalink / raw)
To: git
In-Reply-To: <20161021223927.26364-1-gitster@pobox.com>
The callchain that originates at transport_print_push_status()
eventually hits a single leaf function, print_ref_status(), that is
used to show from what old object to what new object a ref got
updated, and the width of the part that shows old and new object
names used a constant TRANSPORT_SUMMARY_WIDTH.
Teach the callchain to pass the width down from the top instead.
This allows a future enhancement to compute the necessary display
width before calling down this callchain.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
transport.c | 63 +++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 38 insertions(+), 25 deletions(-)
diff --git a/transport.c b/transport.c
index 94d6dc3725..ec02b78924 100644
--- a/transport.c
+++ b/transport.c
@@ -295,7 +295,9 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
}
}
-static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg, int porcelain)
+static void print_ref_status(char flag, const char *summary,
+ struct ref *to, struct ref *from, const char *msg,
+ int porcelain, int summary_width)
{
if (porcelain) {
if (from)
@@ -307,7 +309,7 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str
else
fprintf(stdout, "%s\n", summary);
} else {
- fprintf(stderr, " %c %-*s ", flag, TRANSPORT_SUMMARY_WIDTH, summary);
+ fprintf(stderr, " %c %-*s ", flag, summary_width, summary);
if (from)
fprintf(stderr, "%s -> %s", prettify_refname(from->name), prettify_refname(to->name));
else
@@ -321,15 +323,16 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str
}
}
-static void print_ok_ref_status(struct ref *ref, int porcelain)
+static void print_ok_ref_status(struct ref *ref, int porcelain, int summary_width)
{
if (ref->deletion)
- print_ref_status('-', "[deleted]", ref, NULL, NULL, porcelain);
+ print_ref_status('-', "[deleted]", ref, NULL, NULL,
+ porcelain, summary_width);
else if (is_null_oid(&ref->old_oid))
print_ref_status('*',
(starts_with(ref->name, "refs/tags/") ? "[new tag]" :
"[new branch]"),
- ref, ref->peer_ref, NULL, porcelain);
+ ref, ref->peer_ref, NULL, porcelain, summary_width);
else {
struct strbuf quickref = STRBUF_INIT;
char type;
@@ -349,12 +352,14 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)
strbuf_add_unique_abbrev(&quickref, ref->new_oid.hash,
DEFAULT_ABBREV);
- print_ref_status(type, quickref.buf, ref, ref->peer_ref, msg, porcelain);
+ print_ref_status(type, quickref.buf, ref, ref->peer_ref, msg,
+ porcelain, summary_width);
strbuf_release(&quickref);
}
}
-static int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain)
+static int print_one_push_status(struct ref *ref, const char *dest, int count,
+ int porcelain, int summary_width)
{
if (!count) {
char *url = transport_anonymize_url(dest);
@@ -364,56 +369,60 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
switch(ref->status) {
case REF_STATUS_NONE:
- print_ref_status('X', "[no match]", ref, NULL, NULL, porcelain);
+ print_ref_status('X', "[no match]", ref, NULL, NULL,
+ porcelain, summary_width);
break;
case REF_STATUS_REJECT_NODELETE:
print_ref_status('!', "[rejected]", ref, NULL,
- "remote does not support deleting refs", porcelain);
+ "remote does not support deleting refs",
+ porcelain, summary_width);
break;
case REF_STATUS_UPTODATE:
print_ref_status('=', "[up to date]", ref,
- ref->peer_ref, NULL, porcelain);
+ ref->peer_ref, NULL, porcelain, summary_width);
break;
case REF_STATUS_REJECT_NONFASTFORWARD:
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
- "non-fast-forward", porcelain);
+ "non-fast-forward", porcelain, summary_width);
break;
case REF_STATUS_REJECT_ALREADY_EXISTS:
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
- "already exists", porcelain);
+ "already exists", porcelain, summary_width);
break;
case REF_STATUS_REJECT_FETCH_FIRST:
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
- "fetch first", porcelain);
+ "fetch first", porcelain, summary_width);
break;
case REF_STATUS_REJECT_NEEDS_FORCE:
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
- "needs force", porcelain);
+ "needs force", porcelain, summary_width);
break;
case REF_STATUS_REJECT_STALE:
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
- "stale info", porcelain);
+ "stale info", porcelain, summary_width);
break;
case REF_STATUS_REJECT_SHALLOW:
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
- "new shallow roots not allowed", porcelain);
+ "new shallow roots not allowed",
+ porcelain, summary_width);
break;
case REF_STATUS_REMOTE_REJECT:
print_ref_status('!', "[remote rejected]", ref,
- ref->deletion ? NULL : ref->peer_ref,
- ref->remote_status, porcelain);
+ ref->deletion ? NULL : ref->peer_ref,
+ ref->remote_status, porcelain, summary_width);
break;
case REF_STATUS_EXPECTING_REPORT:
print_ref_status('!', "[remote failure]", ref,
- ref->deletion ? NULL : ref->peer_ref,
- "remote failed to report status", porcelain);
+ ref->deletion ? NULL : ref->peer_ref,
+ "remote failed to report status",
+ porcelain, summary_width);
break;
case REF_STATUS_ATOMIC_PUSH_FAILED:
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
- "atomic push failed", porcelain);
+ "atomic push failed", porcelain, summary_width);
break;
case REF_STATUS_OK:
- print_ok_ref_status(ref, porcelain);
+ print_ok_ref_status(ref, porcelain, summary_width);
break;
}
@@ -427,25 +436,29 @@ void transport_print_push_status(const char *dest, struct ref *refs,
int n = 0;
unsigned char head_sha1[20];
char *head;
+ int summary_width = TRANSPORT_SUMMARY_WIDTH;
head = resolve_refdup("HEAD", RESOLVE_REF_READING, head_sha1, NULL);
if (verbose) {
for (ref = refs; ref; ref = ref->next)
if (ref->status == REF_STATUS_UPTODATE)
- n += print_one_push_status(ref, dest, n, porcelain);
+ n += print_one_push_status(ref, dest, n,
+ porcelain, summary_width);
}
for (ref = refs; ref; ref = ref->next)
if (ref->status == REF_STATUS_OK)
- n += print_one_push_status(ref, dest, n, porcelain);
+ n += print_one_push_status(ref, dest, n,
+ porcelain, summary_width);
*reject_reasons = 0;
for (ref = refs; ref; ref = ref->next) {
if (ref->status != REF_STATUS_NONE &&
ref->status != REF_STATUS_UPTODATE &&
ref->status != REF_STATUS_OK)
- n += print_one_push_status(ref, dest, n, porcelain);
+ n += print_one_push_status(ref, dest, n,
+ porcelain, summary_width);
if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {
if (head != NULL && !strcmp(head, ref->name))
*reject_reasons |= REJECT_NON_FF_HEAD;
--
2.10.1-723-g2384e83bc3
^ permalink raw reply related
* [PATCH 3/3] transport: allow summary-width to be computed dynamically
From: Junio C Hamano @ 2016-10-21 22:39 UTC (permalink / raw)
To: git
In-Reply-To: <20161021223927.26364-1-gitster@pobox.com>
Now we have identified three callchains that have a set of refs that
they want to show their <old, new> object names in an aligned output,
we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH
with a helper function call to transport_summary_width() that takes
the set of ref as a parameter. This step does not yet iterate over
the refs and compute, which is left as an exercise to the readers.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/fetch.c | 4 ++--
transport.c | 7 ++++++-
transport.h | 2 +-
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 40696e5338..09813cd826 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -722,7 +722,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
char *url;
const char *filename = dry_run ? "/dev/null" : git_path_fetch_head();
int want_status;
- int summary_width = TRANSPORT_SUMMARY_WIDTH;
+ int summary_width = transport_summary_width(ref_map);
fp = fopen(filename, "a");
if (!fp)
@@ -906,7 +906,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
int url_len, i, result = 0;
struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, ref_map);
char *url;
- int summary_width = TRANSPORT_SUMMARY_WIDTH;
+ int summary_width = transport_summary_width(stale_refs);
const char *dangling_msg = dry_run
? _(" (%s will become dangling)")
: _(" (%s has become dangling)");
diff --git a/transport.c b/transport.c
index ec02b78924..d4b8bf5f25 100644
--- a/transport.c
+++ b/transport.c
@@ -429,6 +429,11 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,
return 1;
}
+int transport_summary_width(const struct ref *refs)
+{
+ return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+}
+
void transport_print_push_status(const char *dest, struct ref *refs,
int verbose, int porcelain, unsigned int *reject_reasons)
{
@@ -436,7 +441,7 @@ void transport_print_push_status(const char *dest, struct ref *refs,
int n = 0;
unsigned char head_sha1[20];
char *head;
- int summary_width = TRANSPORT_SUMMARY_WIDTH;
+ int summary_width = transport_summary_width(refs);
head = resolve_refdup("HEAD", RESOLVE_REF_READING, head_sha1, NULL);
diff --git a/transport.h b/transport.h
index e783377e40..706d99e818 100644
--- a/transport.h
+++ b/transport.h
@@ -142,7 +142,7 @@ struct transport {
#define TRANSPORT_PUSH_ATOMIC 8192
#define TRANSPORT_PUSH_OPTIONS 16384
-#define TRANSPORT_SUMMARY_WIDTH (2 * FALLBACK_DEFAULT_ABBREV + 3)
+extern int transport_summary_width(const struct ref *refs);
/* Returns a transport suitable for the url */
struct transport *transport_get(struct remote *, const char *);
--
2.10.1-723-g2384e83bc3
^ permalink raw reply related
* Re: generating combined diff without an existing merge commit
From: Junio C Hamano @ 2016-10-21 22:41 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xoTO6MqdBjekbSpSs=w_dJ-1h_ypMDAo50mm95iTOo9Pw@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> Turns out that somehow I must have messed up my command because "git
> diff <treeish> <treeish> <treeish>" does indeed do exactly what I
> want.
Thanks. I was wondering why it didn't work for you, as the feature
was done exactly to support that use case ;-)
^ permalink raw reply
* Re: [PATCH] doc: fix merge-base ASCII art tab spacing
From: Junio C Hamano @ 2016-10-21 22:54 UTC (permalink / raw)
To: Philip Oakley; +Cc: GitList, Jeff King, Johannes Schindelin
In-Reply-To: <D861234B3E78496DBA70EE63B2BCDB96@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
> It appears that acciidoctor sees the art as being a separated
> mono-spaced block, with border/background as locally
> appropriate. While the asciidoc looks to simply change to mono-spaced
> text.
FWIW, I changed my mind and your patch is now queued on 'next'.
Thanks.
^ permalink raw reply
* Re: generating combined diff without an existing merge commit
From: Jacob Keller @ 2016-10-21 22:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list
In-Reply-To: <xmqq60olb9zq.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 21, 2016 at 3:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> Turns out that somehow I must have messed up my command because "git
>> diff <treeish> <treeish> <treeish>" does indeed do exactly what I
>> want.
>
> Thanks. I was wondering why it didn't work for you, as the feature
> was done exactly to support that use case ;-)
I'm not really sure. It kept spitting out usage information to me, but
then I tried again with a fresh example and it worked as expected.
Thanks,
Jake
^ permalink raw reply
* [PATCH 0/3] Fix submodule url issues
From: Stefan Beller @ 2016-10-21 23:59 UTC (permalink / raw)
To: gitster, j6t, Johannes.Schindelin
Cc: git, venv21, dennis, jrnieder, Stefan Beller
previous patch: http://public-inbox.org/git/20161018210623.32696-1-sbeller@google.com/
First fix our test suite in patch 2,
then patch 3 is a resend of the previous patch to normalize the configured
remote.
Thanks,
Stefan
Stefan Beller (3):
t7506: fix diff order arguments in test_cmp
submodule tests: replace cloning from . by "$(pwd)"
submodule--helper: normalize funny urls
builtin/submodule--helper.c | 48 +++++++++++++++++++++++++++++++++-----------
t/t0060-path-utils.sh | 11 ++++++----
t/t3600-rm.sh | 1 +
t/t7064-wtstatus-pv2.sh | 9 ++++++---
t/t7403-submodule-sync.sh | 3 ++-
t/t7406-submodule-update.sh | 6 ++++--
t/t7407-submodule-foreach.sh | 3 ++-
t/t7506-status-submodule.sh | 7 ++++---
8 files changed, 62 insertions(+), 26 deletions(-)
--
2.10.1.507.g2a9098a
^ permalink raw reply
* Re: [PATCH v5 0/8] allow non-trailers and multiple-line trailers
From: Junio C Hamano @ 2016-10-21 23:59 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Git Mailing List, Stefan Beller
In-Reply-To: <cover.1477072247.git.jonathantanmy@google.com>
On Fri, Oct 21, 2016 at 10:54 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
> I've updated patch 5/8 to use strcspn and to pass in the list of
> separators, meaning that we no longer accept '=' in file input (and also
> updated its commit message accordingly).
Thanks for a pleasant read. Queued.
Hopefully this is ready for 'next' now.
^ permalink raw reply
* [PATCH 1/3] t7506: fix diff order arguments in test_cmp
From: Stefan Beller @ 2016-10-21 23:59 UTC (permalink / raw)
To: gitster, j6t, Johannes.Schindelin
Cc: git, venv21, dennis, jrnieder, Stefan Beller
In-Reply-To: <20161021235939.20792-1-sbeller@google.com>
Fix the argument order for test_cmp. When given the expected
result first the diff shows the actual output with '+' and the
expectation with '-', which is the convention for our tests.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/t7506-status-submodule.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d31b34d..74cb6b8 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -260,7 +260,7 @@ test_expect_success 'diff with merge conflict in .gitmodules' '
cd super &&
git diff >../diff_actual 2>&1
) &&
- test_cmp diff_actual diff_expect
+ test_cmp diff_expect diff_actual
'
test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
@@ -268,7 +268,7 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
cd super &&
git diff --submodule >../diff_submodule_actual 2>&1
) &&
- test_cmp diff_submodule_actual diff_submodule_expect
+ test_cmp diff_submodule_expect diff_submodule_actual
'
test_done
--
2.10.1.507.g2a9098a
^ permalink raw reply related
* [PATCH 3/3] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-21 23:59 UTC (permalink / raw)
To: gitster, j6t, Johannes.Schindelin
Cc: git, venv21, dennis, jrnieder, Stefan Beller
In-Reply-To: <20161021235939.20792-1-sbeller@google.com>
The remote URL for the submodule can be specified relative
to the URL of the superproject in .gitmodules. A top-level
git://site.xz/toplevel.git can specify in its .gitmodules
[submodule "sub"]
url = ../submodule.git
path = sub
to say that git://site.xz/submodule.git is where the
submodule bound at its "sub/" is found.
However, when the toplevel is cloned like this:
git clone git://site.xz/toplevel.git/. top
i.e. the toplevel specifies its URL with trailing "/.", the
code set the URL to git://site.xz/toplevel.git/submodule.git
for the submodule, which is nonsense. This was because the
logic failed to treat trailing "/." any differently from
trailing "/<anything-without-slash>" when resolving a
relative URL "../<something>" off of it. Stripping "/." at
the end does *not* take you one level up, even though
stripping "/<anything-without-slash>" does!
Some users may rely on this by always cloning with '/.' and having
an additional '../' in the relative path for the submodule, and this
patch breaks them. So why introduce this patch?
The fix in c12922024 (submodule: ignore trailing slash on superproject
URL, 2016-10-10) and prior discussion revealed, that Git and Git
for Windows treat URLs differently, as currently Git for Windows
strips off a trailing dot from paths when calling a Git binary
unlike when running a shell. Which means Git for Windows is already
doing the right thing for the case mentioned above, but it would fail
our current tests, that test for the broken behavior and it would
confuse users working across platforms. So we'd rather fix it
in Git to ignore any of these trailing no ops in the path properly.
We never produce the URLs with a trailing '/.' in Git ourselves,
they come to us, because the user used it as the URL for cloning
a superproject. Normalize these paths.
The test 3600 needs a slight adaption as well, and was not covered by
the previous patch, because the setup of the submodule in this test
is different than the "clone . super" pattern that was fixed in the
previous patch.
The url configured for the submodule path submod is "./.", which
gets normalized with this patch, such that the nested submodule
'subsubmod' doesn't resolve its path properly via '../'.
In later tests we do not need the url any more as testing of
removal of the config including url happens in early tests, so the
easieast fix for that test is to just make the submodule its own
authoritative source of truth by removing the remote url.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/submodule--helper.c | 48 +++++++++++++++++++++++++++++++++------------
t/t0060-path-utils.sh | 11 +++++++----
t/t3600-rm.sh | 1 +
3 files changed, 44 insertions(+), 16 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..21e2e2a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -76,6 +76,29 @@ static int chop_last_dir(char **remoteurl, int is_relative)
return 0;
}
+static void strip_url_ending(char *url, size_t *len_)
+{
+ size_t len = len_ ? *len_ : strlen(url);
+
+ for (;;) {
+ if (len > 1 && is_dir_sep(url[len - 2]) && url[len - 1] == '.') {
+ url[len-2] = '\0';
+ len -= 2;
+ continue;
+ }
+ if (len > 0 && is_dir_sep(url[len-1])) {
+ url[len-1] = '\0';
+ len--;
+ continue;
+ }
+
+ break;
+ }
+
+ if (len_)
+ *len_ = len;
+}
+
/*
* The `url` argument is the URL that navigates to the submodule origin
* repo. When relative, this URL is relative to the superproject origin
@@ -93,14 +116,16 @@ static int chop_last_dir(char **remoteurl, int is_relative)
* the superproject working tree otherwise.
*
* NEEDSWORK: This works incorrectly on the domain and protocol part.
- * remote_url url outcome expectation
- * http://a.com/b ../c http://a.com/c as is
- * http://a.com/b/ ../c http://a.com/c same as previous line, but
- * ignore trailing slash in url
- * http://a.com/b ../../c http://c error out
- * http://a.com/b ../../../c http:/c error out
- * http://a.com/b ../../../../c http:c error out
- * http://a.com/b ../../../../../c .:c error out
+ * remote_url url outcome expectation
+ * http://a.com/b ../c http://a.com/c as is
+ * http://a.com/b/ ../c http://a.com/c same as previous line, but
+ * ignore trailing '/' in url
+ * http://a.com/b/. ../c http://a.com/c same as previous line, but
+ * ignore trailing '/.' in url
+ * http://a.com/b ../../c http://c error out
+ * http://a.com/b ../../../c http:/c error out
+ * http://a.com/b ../../../../c http:c error out
+ * http://a.com/b ../../../../../c .:c error out
* NEEDSWORK: Given how chop_last_dir() works, this function is broken
* when a local part has a colon in its path component, too.
*/
@@ -115,8 +140,7 @@ static char *relative_url(const char *remote_url,
struct strbuf sb = STRBUF_INIT;
size_t len = strlen(remoteurl);
- if (is_dir_sep(remoteurl[len-1]))
- remoteurl[len-1] = '\0';
+ strip_url_ending(remoteurl, &len);
if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
is_relative = 0;
@@ -149,10 +173,10 @@ static char *relative_url(const char *remote_url,
}
strbuf_reset(&sb);
strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
- if (ends_with(url, "/"))
- strbuf_setlen(&sb, sb.len - 1);
free(remoteurl);
+ strip_url_ending(sb.buf, &sb.len);
+
if (starts_with_dot_slash(sb.buf))
out = xstrdup(sb.buf + 2);
else
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 25b48e5..e154e5f 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -329,14 +329,17 @@ test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
-test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
-test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)/."
+test_submodule_relative_url "(null)" "$PWD/sub/." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD/sub/./." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD/sub/.////././/./." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)"
test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"
test_submodule_relative_url "(null)" "$PWD" "./å äö" "$(pwd)/å äö"
-test_submodule_relative_url "(null)" "$PWD/." "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$PWD/sub" "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$PWD/sub/." "../submodule" "$(pwd)/submodule"
test_submodule_relative_url "(null)" "$PWD/submodule" "../submodule" "$(pwd)/submodule"
test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
-test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
+test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo"
test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tmp/subrepo"
test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index d046d98..7839ccd 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -615,6 +615,7 @@ test_expect_success 'setup subsubmodule' '
git reset --hard &&
git submodule update &&
(cd submod &&
+ git config --unset remote.origin.url &&
git update-index --add --cacheinfo 160000 $(git rev-parse HEAD) subsubmod &&
git config -f .gitmodules submodule.sub.url ../. &&
git config -f .gitmodules submodule.sub.path subsubmod &&
--
2.10.1.507.g2a9098a
^ permalink raw reply related
* [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Stefan Beller @ 2016-10-21 23:59 UTC (permalink / raw)
To: gitster, j6t, Johannes.Schindelin
Cc: git, venv21, dennis, jrnieder, Stefan Beller
In-Reply-To: <20161021235939.20792-1-sbeller@google.com>
When adding a submodule via "git submodule add <relative url>",
the relative url applies to the superprojects remote. When the
superproject was cloned via "git clone . super", the remote url
is ending with '/.'.
The logic to construct the relative urls is not smart enough to
detect that the ending /. is referring to the directory itself
but rather treats it like any other relative path, i.e.
path/to/dir/. + ../relative/path/to/submodule
would result in
path/to/dir/relative/path/to/submodule
and not omit the "dir" as you may expect.
As in a later patch we'll normalize the remote url before the
computation of relative urls takes place, we need to first get our
test suite in a shape with normalized urls only, which is why we should
refrain from cloning from '.'
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/t7064-wtstatus-pv2.sh | 9 ++++++---
t/t7403-submodule-sync.sh | 3 ++-
t/t7406-submodule-update.sh | 6 ++++--
t/t7407-submodule-foreach.sh | 3 ++-
t/t7506-status-submodule.sh | 3 ++-
5 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 3012a4d..95514bb 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -330,7 +330,8 @@ test_expect_success 'verify UU (edit-edit) conflict' '
test_expect_success 'verify upstream fields in branch header' '
git checkout master &&
test_when_finished "rm -rf sub_repo" &&
- git clone . sub_repo &&
+ git clone "$(pwd)" sub_repo &&
+ git -C sub_repo config --unset remote.origin.url &&
(
## Confirm local master tracks remote master.
cd sub_repo &&
@@ -392,8 +393,10 @@ test_expect_success 'verify upstream fields in branch header' '
test_expect_success 'create and add submodule, submodule appears clean (A. S...)' '
git checkout master &&
- git clone . sub_repo &&
- git clone . super_repo &&
+ git clone "$(pwd)" sub_repo &&
+ git -C sub_repo config --unset remote.origin.url &&
+ git clone "$(pwd)" super_repo &&
+ git -C super_repo config --unset remote.origin.url &&
( cd super_repo &&
git submodule add ../sub_repo sub1 &&
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 0726799..6d85391 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -15,7 +15,8 @@ test_expect_success setup '
git add file &&
test_tick &&
git commit -m upstream &&
- git clone . super &&
+ git clone "$(pwd)" super &&
+ git -C super config --unset remote.origin.url &&
git clone super submodule &&
(
cd submodule &&
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 64f322c..9430c2a 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -26,7 +26,8 @@ test_expect_success 'setup a submodule tree' '
git add file &&
test_tick &&
git commit -m upstream &&
- git clone . super &&
+ git clone "$(pwd)" super &&
+ git -C super config --unset remote.origin.url &&
git clone super submodule &&
git clone super rebasing &&
git clone super merging &&
@@ -64,7 +65,8 @@ test_expect_success 'setup a submodule tree' '
test_tick &&
git commit -m "none"
) &&
- git clone . recursivesuper &&
+ git clone "$(pwd)" recursivesuper &&
+ git -C recursivesuper config --unset remote.origin.url &&
( cd recursivesuper
git submodule add ../super super
)
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf..257e817 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -17,7 +17,8 @@ test_expect_success 'setup a submodule tree' '
git add file &&
test_tick &&
git commit -m upstream &&
- git clone . super &&
+ git clone "$(pwd)" super &&
+ git -C super config --unset remote.origin.url &&
git clone super submodule &&
(
cd super &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 74cb6b8..62a99bc 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -197,7 +197,8 @@ A sub1
EOF
test_expect_success 'status with merge conflict in .gitmodules' '
- git clone . super &&
+ git clone "$(pwd)" super &&
+ git -C super config --unset remote.origin.url &&
test_create_repo_with_commit sub1 &&
test_tick &&
test_create_repo_with_commit sub2 &&
--
2.10.1.507.g2a9098a
^ permalink raw reply related
* Re: [PATCH v5 0/8] allow non-trailers and multiple-line trailers
From: Stefan Beller @ 2016-10-22 0:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, Git Mailing List
In-Reply-To: <CAPc5daX-bdSBAxy60zG2ZuGbrjGwRcsvHFktCqKw_o2QuuWTEg@mail.gmail.com>
On Fri, Oct 21, 2016 at 4:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> On Fri, Oct 21, 2016 at 10:54 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>> I've updated patch 5/8 to use strcspn and to pass in the list of
>> separators, meaning that we no longer accept '=' in file input (and also
>> updated its commit message accordingly).
>
> Thanks for a pleasant read. Queued.
>
> Hopefully this is ready for 'next' now.
I also just read through and was about to say the same.
^ permalink raw reply
* Re: [PATCH] doc: fix merge-base ASCII art tab spacing
From: Jeff King @ 2016-10-22 1:09 UTC (permalink / raw)
To: Philip Oakley; +Cc: Junio C Hamano, GitList, Johannes Schindelin
In-Reply-To: <D861234B3E78496DBA70EE63B2BCDB96@PhilipOakley>
On Fri, Oct 21, 2016 at 10:26:29PM +0100, Philip Oakley wrote:
> > updating the source they work on. Otherwise, the broken "doc-tool
> > stack" will keep producing broken output next time a source that
> > respects "tab is to skip to the next multiple of 8" rule is fed to
> > it, no?
>
> By avoiding tabs *within the art* we would also be tolerant of those who may
> not have a set their tab spacing to 8 when viewing the raw text.
>
> It's particularly the criss-cross diagram that needs fixed one way or
> another (for the doc/doctor differences).
I think the new asciidoctor correctly handles tabs within the art. The
earlier diagrams begin each line with a tab (to mark the pre-formatted
block), and then only some of the lines have additional tabs, and expect
those tabs to expand to 8 characters to line up with the other bits
(which is what caused a problem with earlier asciidoctor).
What is funny about that criss-cross diagram is that it actually chooses
different markers on each line to start the art: sometimes tabs and
sometimes spaces. And that seems to confuse even recent versions of
asciidoctor.
It may be that asciidoctor is wrong here, but I have to admit we are
venturing well into "what happens to work with asciidoc" territory, and
the right solution is just fixing the diagram (i.e., your patch).
-Peff
^ permalink raw reply
* Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths
From: Jeff King @ 2016-10-22 3:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Luke Shumaker, Git Mailing List
In-Reply-To: <xmqqvawlblxi.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 21, 2016 at 11:23:53AM -0700, Junio C Hamano wrote:
> A request to "git://<site>/<string>", depending on <string>, results
> in "directory" given to path_ok() in a bit different forms. Namely,
> connect.c::parse_connect_url() gives
>
> URL directory
> git://site/nouser.git /nouser.git
> git://site/~nouser.git ~nouser.git
>
> by special casing "~user" syntax (in other words, a pathname that
> begins with a tilde _is_ special cased, and tilde is not considered
> a normal character that can be in a pathname). Note the lack of
> leading slash in the second one.
>
> Because that is how the shipped clients work, the daemon needs a bit
> of adjustment, because interpolation and base-path prefix codepaths
> wants to accept only paths that begin with a slash, and relies on
> the slash when interpolating it in the template or concatenating it
> to the base (e.g. roughly "sprintf(buf, "%s%s", base_path, dir)").
>
> I came up with the following as a less invasive patch. There no
> longer is the ase where "user-path not allowed" error is given,
> as treating tilde as just a normal pathname character even when it
> appears at the beginning is the whole point of this change.
Thanks for explaining this. It is rather gross, but I think your
less-invasive patch is the best we could do given the client behavior.
And it's more what I would have expected based on the original problem
description.
> As I said already, I am not sure if this is a good change, though.
I also am on the fence. I'm not sure I understand a compelling reason to
have a non-interpolated "~" in the repo path name. Sure, it's a
limitation, but why does anybody care?
> @@ -170,11 +171,11 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
> return NULL;
> }
>
> + if (!user_path && *dir == '~') {
> + snprintf(tilde_path, PATH_MAX, "/%s", dir);
> + dir = tilde_path;
> + }
I know you are following existing convention in this function to use an
unchecked snprintf(), but it makes me wonder what kind of mischief a
client could get up to by silently truncating via snprintf. This
function is, after all, supposed to be checking the quality of the
incoming path.
xsnprintf() is probably too blunt a hammer, but:
if (snprintf(rpath, PATH_MAX, ...) >= PATH_MAX) {
logerror("path too long");
return NULL;
}
in various places may be appropriate.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically
From: Jeff King @ 2016-10-22 4:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20161021223927.26364-4-gitster@pobox.com>
On Fri, Oct 21, 2016 at 03:39:27PM -0700, Junio C Hamano wrote:
> Now we have identified three callchains that have a set of refs that
> they want to show their <old, new> object names in an aligned output,
> we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH
> with a helper function call to transport_summary_width() that takes
> the set of ref as a parameter. This step does not yet iterate over
> the refs and compute, which is left as an exercise to the readers.
The final step could be something like this:
diff --git a/transport.c b/transport.c
index 4dac713063..c1eaa4a 100644
--- a/transport.c
+++ b/transport.c
@@ -443,7 +443,21 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,
int transport_summary_width(const struct ref *refs)
{
- return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+ int max_abbrev;
+
+ /*
+ * Computing the complete set of abbreviated sha1s is expensive just to
+ * find their lengths, but we can at least find our real dynamic
+ * minimum by picking an arbitrary sha1.
+ */
+ if (refs)
+ max_abbrev = strlen(find_unique_abbrev(refs->old_oid.hash,
+ DEFAULT_ABBREV));
+ else
+ max_abbrev = FALLBACK_DEFAULT_ABBREV;
+
+ /* 2 abbreviated sha1s, plus "..." in between */
+ return (2 * max_abbrev + 3);
}
void transport_print_push_status(const char *dest, struct ref *refs,
which produces reasonable results for me. But if we really wanted the
true value, I think we'd want to compute and store the abbreviated
sha1s, and then the refactoring done by your series probably isn't the
right direction.
I think we'd instead want to replace "struct strbuf *display" passed
down to update_local_ref() with something more like:
struct ref_status_table {
struct ref_status_item {
char old_hash[GIT_SHA1_HEX + 1];
char new_hash[GIT_SHA1_HEX + 1];
char *remote_ref;
char *local_ref;
char *summary;
char code;
} *items;
size_t alloc, nr;
};
and then format_display() would just add to the list (including calling
find_unique_abbrev()), and then at the end we'd call a function to show
them all.
That would also get rid of prepare_format_display(), as we could easily
walk over the prepared list before printing any of them (as opposed to
what that function does now, which is to walk over the ref map; that
requires that it know which refs are going to be printed).
-Peff
^ permalink raw reply related
* Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically
From: Junio C Hamano @ 2016-10-22 4:39 UTC (permalink / raw)
To: git
In-Reply-To: <20161021223927.26364-4-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> Now we have identified three callchains that have a set of refs that
> they want to show their <old, new> object names in an aligned output,
> we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH
> with a helper function call to transport_summary_width() that takes
> the set of ref as a parameter. This step does not yet iterate over
> the refs and compute, which is left as an exercise to the readers.
And this is the final one.
-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 21 Oct 2016 21:33:06 -0700
Subject: [PATCH] transport: compute summary-width dynamically
Now all that is left to do is to actually iterate over the refs
and measure the display width needed to show their abbreviation.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
transport.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/transport.c b/transport.c
index d4b8bf5f25..f1f95cf7c7 100644
--- a/transport.c
+++ b/transport.c
@@ -429,9 +429,25 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,
return 1;
}
+static int measure_abbrev(const struct object_id *oid, int sofar)
+{
+ char hex[GIT_SHA1_HEXSZ + 1];
+ int w = find_unique_abbrev_r(hex, oid->hash, DEFAULT_ABBREV);
+
+ return (w < sofar) ? sofar : w;
+}
+
int transport_summary_width(const struct ref *refs)
{
- return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+ int maxw;
+
+ for (maxw = -1; refs; refs = refs->next) {
+ maxw = measure_abbrev(&refs->old_oid, maxw);
+ maxw = measure_abbrev(&refs->new_oid, maxw);
+ }
+ if (maxw < 0)
+ maxw = FALLBACK_DEFAULT_ABBREV;
+ return (2 * maxw + 3);
}
void transport_print_push_status(const char *dest, struct ref *refs,
--
2.10.1-723-g2384e83bc3
^ permalink raw reply related
* [PATCH 4/3] test-lib: bail out when "-v" used under "prove"
From: Jeff King @ 2016-10-22 4:45 UTC (permalink / raw)
To: Jacob Keller; +Cc: Junio C Hamano, Stefan Beller, Lars Schneider, git
In-Reply-To: <CA+P7+xp8TDL59tQgCpmMDJ-BspA1CF6YsnuGMXas1bx_X0qJoA@mail.gmail.com>
On Fri, Oct 21, 2016 at 09:15:28AM -0700, Jacob Keller wrote:
> >> > For $HARNESS_ACTIVE with _just_ "--verbose", I don't think it would be a
> >> > good idea to activate it. We should either silently ignore --verbose
> >> > then, or complain and die.
> >>
> >> We should probably do that to make sure people realize what might
> >> happen. I read your series and it has a good explanation of the
> >> possible alternatives. I like the approach you chose.
> >
> > Thanks. Do you want to make a patch on top of my series?
>
> I am not sure I will find time to do it today, so it wouldn't be for a
> few more days.
I thought I'd just knock this out in 5 minutes before I forgot about it.
But as with so many things, getting it right proved slightly harder than
I thought. But I did learn about TAP's "Bail out!" directive. And
apparently you can pass it back arbitrary YAML (!). And the "--verbose"
output really is violating the spec, and they claim that Test::Harness
will eventually be tightened to complain (though that was in 2007, and
it still hasn't happened, so...).
Anyway. Here is the patch I came up with (on top of the others).
-- >8 --
Subject: test-lib: bail out when "-v" used under "prove"
When there is a TAP harness consuming the output of our test
scripts, the "--verbose" breaks the output by mingling
test command output with TAP. Because the TAP::Harness
module used by "prove" is fairly lenient, this _usually_
works, but it violates the spec, and things get very
confusing if the commands happen to output a line that looks
like TAP (e.g., the word "ok" on its own line).
Let's detect this situation and complain. Just calling
error() isn't great, though; prove will tell us that the
script failed, but the message doesn't make it through to
the user. Instead, we can use the special TAP signal "Bail
out!". This not only shows the message to the user, but
instructs the harness to stop running the tests entirely.
This is exactly what we want here, as the problem is in the
command-line options, and every test script would produce
the same error.
The result looks like this (the first "Bailout called" line
is in red if prove uses color on your terminal):
$ make GIT_TEST_OPTS='--verbose --tee'
rm -f -r 'test-results'
*** prove ***
Bailout called. Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log
FAILED--Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log
Makefile:39: recipe for target 'prove' failed
make: *** [prove] Error 255
Signed-off-by: Jeff King <peff@peff.net>
---
t/test-lib.sh | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 85946ec40d..b859db61ac 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -321,6 +321,16 @@ say () {
say_color info "$*"
}
+if test -n "$HARNESS_ACTIVE"
+then
+ if test "$verbose" = t || test -n "$verbose_only"
+ then
+ printf 'Bail out! %s\n' \
+ 'verbose mode forbidden under TAP harness; try --verbose-log'
+ exit 1
+ fi
+fi
+
test "${test_description}" != "" ||
error "Test script did not set test_description."
--
2.10.1.776.ge0e381e
^ permalink raw reply related
* [PATCH] daemon: detect and reject too-long paths
From: Jeff King @ 2016-10-22 4:59 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
When we are checking the path via path_ok(), we use some
fixed PATH_MAX buffers. We write into them via snprintf(),
so there's no possibility of overflow, but it does mean we
may silently truncate the path, leading to potentially
confusing errors when the partial path does not exist.
We're better off to reject the path explicitly.
Signed-off-by: Jeff King <peff@peff.net>
---
Another option would be to switch to strbufs here. That potentially
introduces cases where a client can convince us to just keep allocating
memory, but I don't think so in practice; the paths and interpolated
data items all have to come in 64K pkt-lines, which places a hard
limit. This is a much more minimal change, though, and I don't hear
anybody complaining about the inability to use large paths.
daemon.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/daemon.c b/daemon.c
index 425aad0507..ff0fa583b0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -160,6 +160,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
{
static char rpath[PATH_MAX];
static char interp_path[PATH_MAX];
+ size_t rlen;
const char *path;
const char *dir;
@@ -187,8 +188,12 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
namlen = slash - dir;
restlen -= namlen;
loginfo("userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
- snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
- namlen, dir, user_path, restlen, slash);
+ rlen = snprintf(rpath, sizeof(rpath), "%.*s/%s%.*s",
+ namlen, dir, user_path, restlen, slash);
+ if (rlen >= sizeof(rpath)) {
+ logerror("user-path too large: %s", rpath);
+ return NULL;
+ }
dir = rpath;
}
}
@@ -207,7 +212,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
strbuf_expand(&expanded_path, interpolated_path,
expand_path, &context);
- strlcpy(interp_path, expanded_path.buf, PATH_MAX);
+
+ rlen = strlcpy(interp_path, expanded_path.buf,
+ sizeof(interp_path));
+ if (rlen >= sizeof(interp_path)) {
+ logerror("interpolated path too large: %s",
+ interp_path);
+ return NULL;
+ }
+
strbuf_release(&expanded_path);
loginfo("Interpolated dir '%s'", interp_path);
@@ -219,7 +232,11 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
logerror("'%s': Non-absolute path denied (base-path active)", dir);
return NULL;
}
- snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
+ rlen = snprintf(rpath, sizeof(rpath), "%s%s", base_path, dir);
+ if (rlen >= sizeof(rpath)) {
+ logerror("base-path too large: %s", rpath);
+ return NULL;
+ }
dir = rpath;
}
--
2.10.1.776.ge0e381e
^ permalink raw reply related
* Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically
From: Jeff King @ 2016-10-22 5:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqoa2d9eum.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote:
> And this is the final one.
>
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Fri, 21 Oct 2016 21:33:06 -0700
> Subject: [PATCH] transport: compute summary-width dynamically
>
> Now all that is left to do is to actually iterate over the refs
> and measure the display width needed to show their abbreviation.
I think we crossed emails. :) This is obviously correct, if we don't
mind paying the find_unique_abbrev cost twice for each sha1.
> int transport_summary_width(const struct ref *refs)
> {
> - return (2 * FALLBACK_DEFAULT_ABBREV + 3);
> + int maxw;
> +
> + for (maxw = -1; refs; refs = refs->next) {
> + maxw = measure_abbrev(&refs->old_oid, maxw);
> + maxw = measure_abbrev(&refs->new_oid, maxw);
> + }
This is a minor style nit, but I think it's better to avoid mixing
unrelated bits between the initialization, condition, and iteration bits
of a for loop. IOW:
int maxw = -1;
for (; refs; refs = refs->next) {
...
}
makes the for-loop _just_ about iteration, and it is more clear that the
manipulation of "maxw" is a side effect of the loop that is valid after
it finishes.
Though in this particular case, the loop and the whole function are
short enough that I don't care that much. Just raising a philosophical
point. :)
-Peff
^ permalink raw reply
* Re: [PATCH 4/3] test-lib: bail out when "-v" used under "prove"
From: Jacob Keller @ 2016-10-22 5:25 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Stefan Beller, Lars Schneider, git
In-Reply-To: <20161022044506.vba6g2q25yxa2air@sigill.intra.peff.net>
On Fri, Oct 21, 2016 at 9:45 PM, Jeff King <peff@peff.net> wrote:
> I thought I'd just knock this out in 5 minutes before I forgot about it.
> But as with so many things, getting it right proved slightly harder than
> I thought.
Always seems to be that way, doesn't it?
> But I did learn about TAP's "Bail out!" directive. And
> apparently you can pass it back arbitrary YAML (!). And the "--verbose"
> output really is violating the spec, and they claim that Test::Harness
> will eventually be tightened to complain (though that was in 2007, and
> it still hasn't happened, so...).
>
> Anyway. Here is the patch I came up with (on top of the others).
>
Nice.
> -- >8 --
> Subject: test-lib: bail out when "-v" used under "prove"
>
> When there is a TAP harness consuming the output of our test
> scripts, the "--verbose" breaks the output by mingling
> test command output with TAP. Because the TAP::Harness
> module used by "prove" is fairly lenient, this _usually_
> works, but it violates the spec, and things get very
> confusing if the commands happen to output a line that looks
> like TAP (e.g., the word "ok" on its own line).
>
> Let's detect this situation and complain. Just calling
> error() isn't great, though; prove will tell us that the
> script failed, but the message doesn't make it through to
> the user. Instead, we can use the special TAP signal "Bail
> out!". This not only shows the message to the user, but
> instructs the harness to stop running the tests entirely.
> This is exactly what we want here, as the problem is in the
> command-line options, and every test script would produce
> the same error.
>
> The result looks like this (the first "Bailout called" line
> is in red if prove uses color on your terminal):
>
> $ make GIT_TEST_OPTS='--verbose --tee'
> rm -f -r 'test-results'
> *** prove ***
> Bailout called. Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log
> FAILED--Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log
> Makefile:39: recipe for target 'prove' failed
> make: *** [prove] Error 255
>
Nice that makes sense.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/test-lib.sh | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 85946ec40d..b859db61ac 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -321,6 +321,16 @@ say () {
> say_color info "$*"
> }
>
> +if test -n "$HARNESS_ACTIVE"
> +then
> + if test "$verbose" = t || test -n "$verbose_only"
> + then
> + printf 'Bail out! %s\n' \
> + 'verbose mode forbidden under TAP harness; try --verbose-log'
> + exit 1
> + fi
> +fi
> +
Not too much code, so that's good. I like it.
Thanks,
Jake
> test "${test_description}" != "" ||
> error "Test script did not set test_description."
>
> --
> 2.10.1.776.ge0e381e
>
^ 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