Git development
 help / color / mirror / Atom feed
* [PATCH 1/8 v6] make lineno_width() from blame reusable for others
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-20 21:57 UTC (permalink / raw)
  To: git, gitster
  Cc: Michael J Gruber, pclouds, j.sixt,
	Zbigniew Jędrzejewski-Szmek
In-Reply-To: <1329775034-21551-1-git-send-email-zbyszek@in.waw.pl>

builtin/blame.c has a helper function to compute how many columns we
need to show a line-number, whose implementation is reusable as a more
generic helper function to count the number of columns necessary to
show any cardinal number.

Rename it to decimal_width(), move it to pager.c and export it for use
by future callers.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
 builtin/blame.c | 18 +++---------------
 cache.h         |  1 +
 pager.c         | 13 +++++++++++++
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git builtin/blame.c builtin/blame.c
index 01956c8..b35bd62 100644
--- builtin/blame.c
+++ builtin/blame.c
@@ -1829,18 +1829,6 @@ static int read_ancestry(const char *graft_file)
 }
 
 /*
- * How many columns do we need to show line numbers in decimal?
- */
-static int lineno_width(int lines)
-{
-	int i, width;
-
-	for (width = 1, i = 10; i <= lines; width++)
-		i *= 10;
-	return width;
-}
-
-/*
  * How many columns do we need to show line numbers, authors,
  * and filenames?
  */
@@ -1880,9 +1868,9 @@ static void find_alignment(struct scoreboard *sb, int *option)
 		if (largest_score < ent_score(sb, e))
 			largest_score = ent_score(sb, e);
 	}
-	max_orig_digits = lineno_width(longest_src_lines);
-	max_digits = lineno_width(longest_dst_lines);
-	max_score_digits = lineno_width(largest_score);
+	max_orig_digits = decimal_width(longest_src_lines);
+	max_digits = decimal_width(longest_dst_lines);
+	max_score_digits = decimal_width(largest_score);
 }
 
 /*
diff --git cache.h cache.h
index 9ecdf76..980d95d 100644
--- cache.h
+++ cache.h
@@ -1198,6 +1198,7 @@ extern const char *pager_program;
 extern int pager_in_use(void);
 extern int pager_use_color;
 extern int term_columns(void);
+extern int decimal_width(uintmax_t number);
 
 extern const char *editor_program;
 extern const char *askpass_program;
diff --git pager.c pager.c
index b790967..60be7bb 100644
--- pager.c
+++ pager.c
@@ -147,3 +147,16 @@ int term_columns(void)
 
 	return term_columns_at_startup;
 }
+
+/*
+ * How many columns do we need to show this number in decimal?
+ */
+int decimal_width(uintmax_t number)
+{
+	int width;
+	uintmax_t i;
+
+	for (width = 1, i = 10; i <= number; width++)
+		i *= 10;
+	return width;
+}
-- 
1.7.9.1.353.g684b4

^ permalink raw reply related

* [PATCH 0/8 v6] diff --stat: use the full terminal width
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-20 21:57 UTC (permalink / raw)
  To: git, gitster
  Cc: Michael J Gruber, pclouds, j.sixt,
	Zbigniew Jędrzejewski-Szmek

Hi,

this is v6, with a new approach. The biggest change is that now the
full terminal width is used in show_stats() only when explictely
requested by the calling code. This is done by setting stat_width=-1
in diffopts in the builtin commands diff, stat, log, and merge. The
series includes to-be-squashed-eds from Junio and is also structured
differently. I followed Junio's advice and split the change to use
full terminal-width, into two patches: one which makes show_stats()
start using term_columns(), and a second one which changes the logic
to divide available columns. This proved to be a good move because it
caught a bug/deficiency in the tests: the tests I added tried to
verify that format-patch output ignores COLUMNS by setting
COLUMNS=200. Unfortunately, because the graph part was limited to 40
columns, the maximum output was less than 80 columns, the test
didn't really check anything. New tests check with both 40 and 200
columns to verify that it really works.

Since there were conflicts with 'jc/diff-stat-scaler' and
'nd/diffstat-gramnum', this re-roll is rebased on top of those two
branches.

v6:

[1/8]  make lineno_width() from blame reusable for others
  This is very close to what was in pu, but I'm sending a new version:
  - the function argument is changed from int to uintmax_t
    (max_change is uintmax_t and 9/9 does decimal_width(max_change).)
[2/8] diff --stat: tests for long filenames and big change counts
  - Tests are run for format-patch, diff, log, show, and merge.
  - Since tests are not only for format-patch, they are added in a new
    file t/t4052-stat-output.sh.
[3/8] diff --stat: use the full terminal width
  Add logic to use term_columns() when diffopts.stat_width==-1 and
  turn it on in git-diff --stat.
  - show_stats() output is adapted to full terminal width only when
    diffopts.stat_width==-1.
[4/8] show --stat: use the full terminal width
  Enable for git-show.
[5/8] log --stat: use the full terminal width
  Enable for log-show.
[6/8] merge --stat: use the full terminal width
  Enable for git-merge.
[7/8] diff --stat: limit graph part to 40 columns
  Change the logic to divide columns. This part is the unchanged from 
  v5, just separated from 3/9.
[8/8] diff --stat: use less columns for change counts
  This one is optional, to be applied or not, "when the dust settles".

Open questions:
JC:
> Perhaps the maximum for garph_width should be raised to something like
> "min(80, stat_width) - name_width"?
I think that a graph like
a | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
b |    1 -
is not very readable. I like the consistency forced by the 40-column limit.
But I guess that this is very subjective.

NTC:
> If the required number of columns is less than 80 (or even lower), can
> we maintain current spacing strategy? I just want to avoid mass
> updates in the test suite. More or less space does not make much
> different for narrow diffstats anyway.
Patch 9/9 does this "massive" change. It touches many files but is
completely trivial. I hope it'll not be a problem.

Zbyszek


v5:
- tests are moved to an earlier patch
  - seq is replaced with a while loop for windows compatibility
  - grep -m 1 is replaced with grep " | "
  - redirects are made portable
  - piped output is split into two commands to verify that the first command
    sucessfully runs to completion
- using decimal_width(change count) is moved to a later patch
- "histogram" is really not used


v4:
- comments are updated and the word "histogram" is banished
- "mopping up" is removed (but the minimum width are guaranteed)

v3:
- use decimal_width(max_change) to calculate number of columns
  required for change counts
- rework the logic to divide columns
- document the logic in comments, update docs
- add more tests

v2:
- style fixes
- some tests for git-format-patch added
- patches 3 and 4 squashed together, since they touch the same lines
- graph width is limited to 40 columns, even if there's more space
- patch descriptions extended and cleared up

 Documentation/diff-options.txt                     |  14 +-
 Documentation/gitcore-tutorial.txt                 |   4 +-
 builtin/blame.c                                    |  18 +-
 builtin/diff.c                                     |   3 +
 builtin/log.c                                      |   3 +
 builtin/merge.c                                    |   1 +
 builtin/reset.c                                    |   2 +
 cache.h                                            |   1 +
 diff.c                                             | 129 ++++++++++----
 pager.c                                            |  13 ++
 t/t0023-crlf-am.sh                                 |   2 +-
 t/t1200-tutorial.sh                                |   4 +-
 t/t3404-rebase-interactive.sh                      |   2 +-
 t/t3903-stash.sh                                   |   4 +-
 t/t4012-diff-binary.sh                             |  19 +++
 ...ff-tree_--cc_--patch-with-stat_--summary_master |   4 +-
 ...diff-tree_--cc_--patch-with-stat_--summary_side |   6 +-
 .../diff.diff-tree_--cc_--patch-with-stat_master   |   4 +-
 .../diff.diff-tree_--cc_--stat_--summary_master    |   4 +-
 t/t4013/diff.diff-tree_--cc_--stat_--summary_side  |   6 +-
 t/t4013/diff.diff-tree_--cc_--stat_master          |   4 +-
 ...pretty=oneline_--root_--patch-with-stat_initial |   6 +-
 .../diff.diff-tree_--pretty_--patch-with-stat_side |   6 +-
 ...-tree_--pretty_--root_--patch-with-stat_initial |   6 +-
 ...f-tree_--pretty_--root_--stat_--summary_initial |   6 +-
 .../diff.diff-tree_--pretty_--root_--stat_initial  |   6 +-
 ...diff.diff-tree_--root_--patch-with-stat_initial |   6 +-
 t/t4013/diff.diff-tree_-c_--stat_--summary_master  |   4 +-
 t/t4013/diff.diff-tree_-c_--stat_--summary_side    |   6 +-
 t/t4013/diff.diff-tree_-c_--stat_master            |   4 +-
 .../diff.diff_--patch-with-stat_-r_initial..side   |   6 +-
 t/t4013/diff.diff_--patch-with-stat_initial..side  |   6 +-
 t/t4013/diff.diff_--stat_initial..side             |   6 +-
 t/t4013/diff.diff_-r_--stat_initial..side          |   6 +-
 ..._--attach_--stdout_--suffix=.diff_initial..side |   6 +-
 ....format-patch_--attach_--stdout_initial..master |  16 +-
 ...format-patch_--attach_--stdout_initial..master^ |  10 +-
 ...ff.format-patch_--attach_--stdout_initial..side |   6 +-
 ...nline_--stdout_--numbered-files_initial..master |  16 +-
 ...tdout_--subject-prefix=TESTCASE_initial..master |  16 +-
 ....format-patch_--inline_--stdout_initial..master |  16 +-
 ...format-patch_--inline_--stdout_initial..master^ |  10 +-
 ...ormat-patch_--inline_--stdout_initial..master^^ |   6 +-
 ...ff.format-patch_--inline_--stdout_initial..side |   6 +-
 ...tch_--stdout_--cover-letter_-n_initial..master^ |  18 +-
 ...at-patch_--stdout_--no-numbered_initial..master |  16 +-
 ...ormat-patch_--stdout_--numbered_initial..master |  16 +-
 t/t4013/diff.format-patch_--stdout_initial..master |  16 +-
 .../diff.format-patch_--stdout_initial..master^    |  10 +-
 t/t4013/diff.format-patch_--stdout_initial..side   |   6 +-
 ....log_--patch-with-stat_--summary_master_--_dir_ |   6 +-
 t/t4013/diff.log_--patch-with-stat_master          |  16 +-
 t/t4013/diff.log_--patch-with-stat_master_--_dir_  |   6 +-
 ..._--root_--cc_--patch-with-stat_--summary_master |  26 +--
 ...f.log_--root_--patch-with-stat_--summary_master |  22 +--
 t/t4013/diff.log_--root_--patch-with-stat_master   |  22 +--
 ...og_--root_-c_--patch-with-stat_--summary_master |  26 +--
 t/t4013/diff.show_--patch-with-stat_--summary_side |   6 +-
 t/t4013/diff.show_--patch-with-stat_side           |   6 +-
 t/t4013/diff.show_--stat_--summary_side            |   6 +-
 t/t4013/diff.show_--stat_side                      |   6 +-
 ...nged_--patch-with-stat_--summary_master_--_dir_ |   6 +-
 t/t4013/diff.whatchanged_--patch-with-stat_master  |  16 +-
 ...ff.whatchanged_--patch-with-stat_master_--_dir_ |   6 +-
 ..._--root_--cc_--patch-with-stat_--summary_master |  26 +--
 ...anged_--root_--patch-with-stat_--summary_master |  22 +--
 ...iff.whatchanged_--root_--patch-with-stat_master |  22 +--
 ...ed_--root_-c_--patch-with-stat_--summary_master |  26 +--
 t/t4014-format-patch.sh                            |   2 +-
 t/t4016-diff-quote.sh                              |  14 +-
 t/t4030-diff-textconv.sh                           |   2 +-
 t/t4043-diff-rename-binary.sh                      |   4 +-
 t/t4045-diff-relative.sh                           |   2 +-
 t/t4047-diff-dirstat.sh                            |  54 +++---
 t/t4049-diff-stat-count.sh                         |   4 +-
 t/t4052-stat-output.sh                             | 189 +++++++++++++++++++++
 t/t5100/patch0001                                  |   2 +-
 t/t5100/patch0002                                  |   2 +-
 t/t5100/patch0003                                  |   2 +-
 t/t5100/patch0005                                  |   4 +-
 t/t5100/patch0006                                  |   2 +-
 t/t5100/patch0010                                  |   2 +-
 t/t5100/patch0011                                  |   2 +-
 t/t5100/patch0014                                  |   2 +-
 t/t5100/patch0014--scissors                        |   2 +-
 t/t5100/sample.mbox                                |  18 +-
 t/t7602-merge-octopus-many.sh                      |  12 +-
 87 files changed, 695 insertions(+), 409 deletions(-)
 create mode 100755 t/t4052-stat-output.sh

-- 
1.7.9.1.354.ge34f3

^ permalink raw reply

* Re: [RFC] pre-rebase: Refuse to rewrite commits that are reachable from upstream
From: Johan Herland @ 2012-02-20 21:21 UTC (permalink / raw)
  To: git; +Cc: Johan Herland, gitster, jnareb, philipoakley
In-Reply-To: <1329772071-11301-1-git-send-email-johan@herland.net>

On Mon, Feb 20, 2012 at 22:07, Johan Herland <johan@herland.net> wrote:
> Teach the pre-rebase sample hook to refuse rewriting commits on a branch
> that are present in that branch's @{upstream}. This is to prevent users
> from rewriting commits that have already been published.
>
> If the branch has no @{upstream}, or the commits-to-be-rebased are not
> reachable from the upstream (hence assumed to be unpublished), the rebase
> is not refused.
>
> This patch is not an ideal solution to the problem, for at least the
> following reasons:
>
>  - There is no way for the user to override this check, except skipping
>   the pre-rebase hook entirely with --no-verify.
>
>  - The check only works for branches with a configured upstream. If the
>   user's workflow does not rely on upstream branches, or uses some other
>   method of publishing commits, the check will produce false negatives
>   (i.e. allow rebases that should have been refused).
>
>  - The check only applies to rebase. I wanted to add the same check
>   on 'commit --amend', but there's no obvious way to detect --amend
>   from within the pre-commit hook.
>
>  - There may be other rewrite scenarios where we want to do this check,
>   such as 'git reset'. Maybe a pre-rewrite hook should be added?
>
>  - Some (including myself) want this check to be performed by default,
>   since it's mostly targeted at newbies that are less likely to enable
>   the pre-rebase (pre-rewrite) hook, so maybe the check should be added
>   to core git instead.
>
> Discussed-with: Jakub Narebski <jnareb@gmail.com>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---

I forgot to explain that this patch is not really submitted for
inclusion, but rather to continue the discussion of getting rewrite
safety properly implemented in git. As such, the problems noted in the
above commit message are probably more important than the patch
itself...

Also, this implements only a small subset of what has been discussed
regarding 'public' and 'secret' properties of commits in the preceding
thread. However, I believe solving this part of the problem
(preventing upstreamed commits from being rewritten) will make 90% of
users happy, and that it's worth fixing on its own merits.


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

^ permalink raw reply

* Re: rfe: git-config: lack of color reset option
From: Jeff King @ 2012-02-20 21:20 UTC (permalink / raw)
  To: Jan Engelhardt; +Cc: git
In-Reply-To: <alpine.LNX.2.01.1202202142160.31585@frira.zrqbmnf.qr>

On Mon, Feb 20, 2012 at 09:50:11PM +0100, Jan Engelhardt wrote:

> given the following config:
> 
> [color "diff"]
> 	commit = bold white blue
> [color "decorate"]
> 	branch = green
> 
> The attributes from color.diff.commit are inherited for color.decorate.

This is an artifact of the way the ANSI colorizing works. Git says "turn
on bold white and a blue background", then it outputs some content, then
it says "turn on green", and so forth. At the end we issue a "reset" to
turn everything back to normal. We should perhaps issue a reset before
outputting the decoration, as we are moving from one colorized bit to
the other, and we don't know what we are inheriting.

Of course that would break people who _want_ the blue background to
continue into the branch decoration. But they can easily fix it by
putting "green blue" in their config.

> 1. There seems to be no way to reset the attributes such that
> "color.decorate.branch = default green blue" wouuld have an effect.

I would have expected that perhaps setting color.decorate.branch to
"reset green" would work, but it seems that we don't allow arbitrary
sequences. Which would be another possible solution.

In your case, I think you just want turn off bold without resetting the
whole thing. That is its own attribute. It would be nice if we supported
"nobold", "noreverse", etc. But you wouldn't really need them if we
properly reset at the transition between two colorized bits.

> 2. It would be nice if there was an option to only paint the 
> commit hash, rather than the entire line including the decorate 
> parenthesis group.

Yeah, the parentheses are explicitly painted. I'm not sure how to easily
fix that short of adding lots of painfully small config options.

I have a long term dream that our --pretty=format specifiers would grow
featureful enough that all of the other --pretty formats could be
implemented in terms of them. And then you could tweak to your hearts
content, starting with the embedded definition of what "git log" shows
and putting colors wherever you like. I'm not sure how far we are off
from doing that now. You could try writing a format-specifier that looks
like git-log output and see if there is anything lacking.

-Peff

^ permalink raw reply

* Re: [PATCH] Support wrapping commit messages when you read them
From: Sidney San Martín @ 2012-02-20 21:09 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzkcmbcbq.fsf@alter.siamese.dyndns.org>

On Feb 13, 2012, at 5:25 PM, Junio C Hamano wrote:

> I just typed M-q to wrap the above paragraph from you to make it readable.

Out of curiosity, how do you read your mail? I don’t know anyone whose mail
is set up like that.

I’m happy to wrap my text if it’s tricky for you to read it otherwise — but
FWIW my mail client doesn’t support hard wrapping (I’m doing it in my editor).

> "Computers are good at automating" is true, and that is why real editors
> give an easy way to auto-wrap long prose in a paragraph while composing.
> But "computers are good at automating" is not a convincing justification
> to let the composer leave unreasonably long lines in the commit log object
> and force the reader side to line-wrap the mess only to fix it up.

I asked in #git how other people handle wrapping. Out of three people who
responded, only one said that they had configured their editor (the other two
do it by hand). One thought that Git already did dumb (character-level) line
wrapping, but it turns out he had set LESS= and GIT_PAGER='less -FRX'.

So, even if it is possible to set up your editor to wrap prose appropriately,
I don’t think it’s as common as one might hope.

I’m not suggesting that the reader side should take care of the wrapping
because it *can*, I’m suggesting that it shouldn’t take specially-configured
editors to get consistent and good results — which I assume is why virtually
all new prose-writing tools do wrapping on the viewing end.

What do you think about Git UIs which use proportional fonts for text where
hard wrapping doesn’t work at all? (I brought this up before but want your
take on it).

And, using man and, now, "git help -a" as examples: they both adapt their
output to the width of the user’s terminal. Isn’t that a good thing?

If those aren’t good justification… what would be?

^ permalink raw reply

* Re: git-subtree Ready #2
From: Jeff King @ 2012-02-20 20:53 UTC (permalink / raw)
  To: David A. Greene; +Cc: git, Avery Pennarun
In-Reply-To: <87ty2ro1zf.fsf@smith.obbligato.org>

On Wed, Feb 15, 2012 at 10:07:16PM -0600, David A. Greene wrote:

> I've attached Avery's response below.  The short summary is that he
> thinks maintaining it in the vger git repository is the way to go and
> that he's fine moving patches to/from GitHub as necessary.
>
> [From Avery:]
>> I'm sure the potential benefit of putting git-subtree in the contrib/
>> directory is that we could then use git-subtree to maintain the
>> git-subtree git subtree, which is a fun wordplay, but perhaps
>> ironically, as a single rarely-changing file, git-subtree is probably
>> not the right tool for these purposes :)

I'm not a git-subtree user, nor am I the maintainer who would pull from
you. So I am somewhat on the sidelines of this particular discussion.

Usually we would incubate new and radically different commands in
contrib, and then if they prove to be good, make first-class commands of
them (e.g., git-new-workdir has been in contrib for a while, and as it
has proven itself to many people, there is talk of including it as a
core command).

My impression is that git-subtree has already done this incubation and
proving step in its own repository (but like I said, I do not use it
myself, so that is just going on list hearsay). So it seems like the
logical step would be to graduate into the main git repository.  And I
gather from Avery's response that he agrees.

Of course there's no real reason we can't take it slow by putting it in
contrib, and then graduating from there. It just seems like an
unnecessary and complicated interim step. Either way, I do think it's
worth saving the commit history by doing a real merge.

I dunno. It is really up to Junio, I guess. He usually relies on list
consensus for decisions like this, and there has not been that much
discussion. What do users of git-subtree think, as this would primarily
benefit them? And what do other members of the git@vger community who do
not use git-subtree think of the burden of carrying it as a first-class
command (not so much the burden of adding it, but of maintaining it,
fielding reports when it is broken, etc)?

As a non-user, I am totally fine with it. I think the burden is not that
high, and you have already promised to deal with ongoing maintenance
issues.

-Peff

^ permalink raw reply

* [PATCH] Ignore SIGPIPE when running a filter driver
From: Jehan Bing @ 2012-02-20 20:53 UTC (permalink / raw)
  To: git; +Cc: gitster, j.sixt, peff, jrnieder, jehan

If a filter is not defined or if it fails, git behaves as if the filter
is a no-op passthru. However, if the filter exits before reading all
the content, and depending on the timing git, could be kill with
SIGPIPE instead.

Ignore SIGPIPE while processing the filter to detect when it exits
early and fallback to using the unfiltered content.

Signed-off-by: Jehan Bing <jehan@orb.com>
---
Since it's not really a problem in the "required-filter" patch but a
general one with filter drivers, I'm submitting this patch
independently. I'm also wording it as a pre-patch to "required-filter".

-Jehan

 convert.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/convert.c b/convert.c
index c06309f..5d312cb 100644
--- a/convert.c
+++ b/convert.c
@@ -2,6 +2,7 @@
 #include "attr.h"
 #include "run-command.h"
 #include "quote.h"
+#include "sigchain.h"
 
 /*
  * convert.c - convert a file when checking it out and checking it in.
@@ -360,12 +361,16 @@ static int filter_buffer(int in, int out, void *data)
 	if (start_command(&child_process))
 		return error("cannot fork to run external filter %s", params->cmd);
 
+	sigchain_push(SIGPIPE, SIG_IGN);
+
 	write_err = (write_in_full(child_process.in, params->src, params->size) < 0);
 	if (close(child_process.in))
 		write_err = 1;
 	if (write_err)
 		error("cannot feed the input to external filter %s", params->cmd);
 
+	sigchain_pop(SIGPIPE);
+
 	status = finish_command(&child_process);
 	if (status)
 		error("external filter %s failed %d", params->cmd, status);
-- 
1.7.9

^ permalink raw reply related

* rfe: git-config: lack of color reset option
From: Jan Engelhardt @ 2012-02-20 20:50 UTC (permalink / raw)
  To: git

Hi,


given the following config:

[color "diff"]
	commit = bold white blue
[color "decorate"]
	branch = green

The attributes from color.diff.commit are inherited for color.decorate.

1. There seems to be no way to reset the attributes such that
"color.decorate.branch = default green blue" wouuld have an effect.

2. It would be nice if there was an option to only paint the 
commit hash, rather than the entire line including the decorate 
parenthesis group.

(My current version is 1.7.7, but there seem to be no changes regarding 
color config in 1.7.8 and 1.7.9 in the release notes, so I presume the 
issue is still current.)

^ permalink raw reply

* Re: git clean is not removing a submodule added to a branch when switching branches
From: Jens Lehmann @ 2012-02-20 20:36 UTC (permalink / raw)
  To: Adrian Cornish; +Cc: git
In-Reply-To: <CAGc=MuDrE_1CVzOsqcodhadcfajaa-BHjHVAp9mFDNbU=wVQqQ@mail.gmail.com>

Am 18.02.2012 22:27, schrieb Adrian Cornish:
> If I add a submodule to a branch and then switch branches, git
> checkout warns it cannot
> remove the submodule. If I then issue a git clean - it says it removes
> the submodule but
> in fact does nothing at all. Is this a bug or expected behaviour.

Right this is just how things work. I won't call it expected behavior,
as some users (like you) would expect to see the submodule be removed
(while others are used to the current behavior that submodule work
trees are never touched).

But one of last years GSoC projects took first steps into the
direction of safely removing submodules (without losing any history).
I have experimental code at my Github repo [1] to update submodule
work trees along with the superproject, but polishing that for
inclusion into mainline will still take some time.

[1] https://github.com/jlehmann/git-submod-enhancements

^ permalink raw reply

* Re: git status: small difference between stating whole repository and small subdirectory
From: Jeff King @ 2012-02-20 20:35 UTC (permalink / raw)
  To: Thomas Rast
  Cc: Nguyen Thai Ngoc Duy, Piotr Krukowiecki, Junio C Hamano,
	Git Mailing List
In-Reply-To: <87d3991gyg.fsf@thomas.inf.ethz.ch>

On Mon, Feb 20, 2012 at 07:45:59PM +0100, Thomas Rast wrote:

> > +	o->result.cache_tree = o->src_index->cache_tree;
> >  	o->src_index = NULL;
> >  	ret = check_updates(o) ? (-2) : 0;
> >  	if (o->dst_index)
> 
> Brilliant.  I know I'm stealing Junio's punchline, but please make it so
> :-)
> 
> Browsing around in history, it seems that this was silently broken by
> 34110cd (Make 'unpack_trees()' have a separate source and destination
> index, 2008-03-06), which introduced the distinction between source and
> destination index.  Before that they were the same, so the cache tree
> would have been updated correctly.

OK, good. When you write a one-liner that makes a huge change in
performance, it is usually a good idea to think to yourself "no, it
couldn't be this easy, could it?".

But after more discussion from people more clueful than I (this is the
first time I've even looked at cache-tree code), I'm feeling like this
is the right direction, at least, if not exactly the right patch.
And seeing that it is in fact a regression in 34110cd, and that the
existing cache-tree invalidations predate that makes me feel better. At
one point, at least, they were complete and we were depending on them to
be accurate. Things may have changed since then, of course, but I at
least know that they were sufficient in 34110cd^.

> +# NEEDSWORK: only one of these two can succeed.  The second is there
> +# because it would be the better result.
> +test_expect_success 'checkout HEAD^ correctly invalidates cache-tree' '
> +	git checkout HEAD^ &&
> +	test_invalid_cache_tree
> +'
> +
> +test_expect_failure 'checkout HEAD^ gives full cache-tree' '
> +	git checkout master &&
> +	git read-tree HEAD &&
>  	git checkout HEAD^ &&
> -	test_shallow_cache_tree
> +	test_cache_tree
>  '

I think you can construct two tests that will both work in the "ideal"
case. In the first one, you move to a tree that updates "foo", and
therefore the root cache-tree is invalidated.

In the second, you update "subdir1/foo" in the index, then move to a
commit that differs in "subdir1/bar" and "subdir2/bar". You should see
that subdir2 has the cache-tree of the destination commit, but that
subdir1 is invalidated (and therefore the root is also invalidated).
That will fail with my patch, of course, as it would invalidate subdir2,
also; so it would just be an expect_failure for somebody in the future.

In general, t0090 could benefit from using a larger tree. For example,
the add test does "git add foo" and checks that the root cache-tree was
invalidated. But it should _also_ check that the cache-tree for a
subdirectory is _not_ invalidated (and it isn't; git-add does the right
thing).

I'll see if I can work up some fancier tests, too.

-Peff

^ permalink raw reply

* Re: git status: small difference between stating whole repository and small subdirectory
From: Jeff King @ 2012-02-20 20:17 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Nguyen Thai Ngoc Duy, Thomas Rast, Piotr Krukowiecki,
	Git Mailing List
In-Reply-To: <7vfwe5l13w.fsf@alter.siamese.dyndns.org>

On Mon, Feb 20, 2012 at 12:08:03PM -0800, Junio C Hamano wrote:

> >   4. At the end of unpack_trees, we forget about src_index, and copy
> >      o->result into *o->dst_index byte for byte. I.e., we overwrite
> >      the_index.cache_tree, which has been properly updated the whole
> >      time,
> 
> I strongly suspect that "properly updated" part needs to be thoroughly
> audited.  I wouldn't be surprised that this behaviour is what we did when
> we split src_index vs dst_index when he rewrote unpack_trees() in order to
> emulate the original "unpack-trees is beyond salvation because it does not
> maintain cache tree correctly, just nuke it" behaviour.

Yep, I am also concerned about that.

> > But it does not actually insert the _destination_ tree into the cache
> > tree. Which we can do in certain situations, but only if there were no
> > paths in the tree that were left unchanged (e.g., you modify "foo", then
> > "git checkout HEAD^", which updates "bar". Your tree does not match
> > HEAD^, and must be invalidated).  While it would be cool to be able to
> > handle those complex cases,...
> 
> It may look cool but it may not be a good change. You are spending extra
> cycles to optimize for the next write-tree that may not happen before the
> index is further updated.

I don't think it would be too many cycles; you would have to mark each
tree you enter as having items from the left-hand tree or the right-hand
tree. If only one, you can reuse the cache-tree entry (or tree sha1, if
coming from a tree). Otherwise, you must invalidate.  And it doesn't
just help the next write-tree, but any intermediate index diffs.

Of course any such change would need timings to justify it, though.

That being said, I think just invalidating really covers 99% of the
cases. What we really care about is that when I modify kernel/foo.c, the
~2300 other directories (besides "" and "kernel") don't need rebuilt,
and that is relatively simple to do. Even if doing it the other way
produced a tiny speedup, I would be concerned with the increase in code
complexity.

> > I think this implementation matches the intent of the original calls to
> > cache_tree_invalidate_path sprinkled throughout unpack-trees.c.
> 
> Yes, and as long as we invalidate all the directories that need to be
> invalidated during the unpack-tree operation, I think it is a correct
> thing to do.

OK. I'll do some reading of the code to convince myself that the
unpack_trees callbacks are doing the right thing. I'm not sure of a good
automatic test that would detect a failure there. Just making test cases
is going to end up too contrived, unless we are missing something really
obvious.

I'm thinking maybe something like replaying the commit history of
linux-2.6 and making sure that each the tree generated by the cache-tree
in each case matches the actual committed tree.

> > But I
> > have to say that it seems a little odd for us to be modifying the
> > o->src_index throughout the whole thing.
> 
> Yes, that part is logically *wrong*.  I think it is a remnant from the
> days when there was no distinction between src_index and dst_index.

OK. I'll include a fix for that in the series I prepare.

-Peff

^ permalink raw reply

* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Junio C Hamano @ 2012-02-20 20:16 UTC (permalink / raw)
  To: Daniel Stenberg; +Cc: Jeff King, Nicolas Mailhot, git
In-Reply-To: <alpine.DEB.2.00.1202202002330.28090@tvnag.unkk.fr>

Daniel Stenberg <daniel@haxx.se> writes:

> As a git user, I would probably be very surprised if using 'git'
> suddenly caused by browser to pop up a captive portal login. I would
> prefer git to instead properly explain to me that is being the victim
> of a 511 and what I should do to fix it.

I agree what you envisioned, nothing more, nothing less, is the ideal
solution.

Thanks.

^ permalink raw reply

* Re: git status: small difference between stating whole repository and small subdirectory
From: Jeff King @ 2012-02-20 20:09 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Piotr Krukowiecki, Thomas Rast, Git Mailing List,
	Nguyen Thai Ngoc Duy
In-Reply-To: <7vr4xpl1nm.fsf@alter.siamese.dyndns.org>

On Mon, Feb 20, 2012 at 11:56:13AM -0800, Junio C Hamano wrote:

> These days, we have src_index and dst_index, and dst_index IIRC can start
> as empty in which case "start from kept information and selectively
> invalidate" would not work at all.  When src_index and dst_index are the
> same, however, you should be able to keep the cached tree valid, at least
> in theory.

Yeah, I was worried that the cache invalidations sprinkled throughout
unpack-trees.c would not be sufficient (and because we are invalidating,
a missing invalidation would give us bogus cache info, which is Very
Bad).

So I think the one-liner I posted before is not sufficient in the
general case, because it definitely doesn't consider where the
destination is starting from. It should at least be more like:

  if (src_index == dst_index) {
          /* We would ordinarily want to do a deep copy here, but since
           * we know that we will be overwriting src_index in the long
           * run, it's OK to just take ownership of its cache_tree. */
          o->result.cache_tree = o->src_index->cache_tree;
          o->src_index->cache_tree = NULL;
  }

  [... do the usual tree traversal here, except invalidate entries in
       o->result.call_tree instead of o->src_index. That makes it a
       no-op when src_index != dst_index (because we have no cache tree
       defined in result, then), and otherwise we are invalidating what
       will go into the result...]

  [then as before, we copy the result to dst_index; except now the
   result may have src_index's cache_tree plus any invalidations]
  o->result = *o->dst_index;

And fortunately that does exactly what we want in all cases, because we
always either read from and write to the_index, or we write to NULL (in
which case we will not bother with a cache_tree for the result, and it
is fixing a minor bug that we might be invalidating src_index's tree in the
first place).

I'm still slightly worried that we are missing some invalidation
somewhere deep in unpack_tree's callbacks (especially because they _are_
callbacks, and invalidating the cache_tree properly is now a promise
that the callbacks have to make).

-Peff

^ permalink raw reply

* Re: git status: small difference between stating whole repository and small subdirectory
From: Junio C Hamano @ 2012-02-20 20:08 UTC (permalink / raw)
  To: Jeff King
  Cc: Nguyen Thai Ngoc Duy, Thomas Rast, Piotr Krukowiecki,
	Junio C Hamano, Git Mailing List
In-Reply-To: <20120220151134.GA13135@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

>   4. At the end of unpack_trees, we forget about src_index, and copy
>      o->result into *o->dst_index byte for byte. I.e., we overwrite
>      the_index.cache_tree, which has been properly updated the whole
>      time,

I strongly suspect that "properly updated" part needs to be thoroughly
audited.  I wouldn't be surprised that this behaviour is what we did when
we split src_index vs dst_index when he rewrote unpack_trees() in order to
emulate the original "unpack-trees is beyond salvation because it does not
maintain cache tree correctly, just nuke it" behaviour.

> But it does not actually insert the _destination_ tree into the cache
> tree. Which we can do in certain situations, but only if there were no
> paths in the tree that were left unchanged (e.g., you modify "foo", then
> "git checkout HEAD^", which updates "bar". Your tree does not match
> HEAD^, and must be invalidated).  While it would be cool to be able to
> handle those complex cases,...

It may look cool but it may not be a good change. You are spending extra
cycles to optimize for the next write-tree that may not happen before the
index is further updated.

> I think this implementation matches the intent of the original calls to
> cache_tree_invalidate_path sprinkled throughout unpack-trees.c.

Yes, and as long as we invalidate all the directories that need to be
invalidated during the unpack-tree operation, I think it is a correct
thing to do.

> But I
> have to say that it seems a little odd for us to be modifying the
> o->src_index throughout the whole thing.

Yes, that part is logically *wrong*.  I think it is a remnant from the
days when there was no distinction between src_index and dst_index.

> I would think the right thing
> would be to make a deep copy of o->src_index->cache_tree into
> o->result.cache_tree as the very first thing, and then update
> o->result.cache_tree throughout the tree traversal.

Yes.

^ permalink raw reply

* Re: git status: small difference between stating whole repository and small subdirectory
From: Thomas Rast @ 2012-02-20 19:59 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jeff King, Piotr Krukowiecki, Git Mailing List,
	Nguyen Thai Ngoc Duy
In-Reply-To: <7vmx8dl1ln.fsf@alter.siamese.dyndns.org>

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

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> test_expect_failure 'checkout gives cache-tree' '
>> 	git checkout HEAD^ &&
>> 	test_shallow_cache_tree
>> '
>
> Depending on what state you start the checkout from, that is not a valid
> test.  Some form of "git reset" before the checkout to ensure the initial
> state is needed.

Oh, I was just quoting what we already had at the end of t0090 since
4eb0346f.  The test preceding it runs 'git reset --hard'.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply

* Re: git status: small difference between stating whole repository and small subdirectory
From: Junio C Hamano @ 2012-02-20 19:57 UTC (permalink / raw)
  To: Thomas Rast
  Cc: Jeff King, Piotr Krukowiecki, Junio C Hamano, Git Mailing List,
	Nguyen Thai Ngoc Duy
In-Reply-To: <87ty2l38ay.fsf@thomas.inf.ethz.ch>

Thomas Rast <trast@inf.ethz.ch> writes:

> test_expect_failure 'checkout gives cache-tree' '
> 	git checkout HEAD^ &&
> 	test_shallow_cache_tree
> '

Depending on what state you start the checkout from, that is not a valid
test.  Some form of "git reset" before the checkout to ensure the initial
state is needed.

^ permalink raw reply

* Re: git status: small difference between stating whole repository and small subdirectory
From: Junio C Hamano @ 2012-02-20 19:56 UTC (permalink / raw)
  To: Jeff King
  Cc: Piotr Krukowiecki, Thomas Rast, Git Mailing List,
	Nguyen Thai Ngoc Duy
In-Reply-To: <20120220140653.GC5131@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Interestingly, on my git.git repo, I had an empty cache. Running "git
> read-tree HEAD" filled it (according to test-dump-cache-tree). It seems
> that running "git checkout" empties the cache.  So perhaps git could do
> better about keeping the cache valid over time.

At least in the early days unpack-trees built the result by manually
adding an entry without calling the add_index_entry() all over the place,
which meant that it was futile to pretend that there is even a slight
chance that complex beast would correctly invalidate cached tree
information at all the necessary places. I recall that I added a code to
nuke the cache tree at the very beginning of "merging" codepaths to avoid
any bogus cache tree result to be stored in the resulting index.

These days, we have src_index and dst_index, and dst_index IIRC can start
as empty in which case "start from kept information and selectively
invalidate" would not work at all.  When src_index and dst_index are the
same, however, you should be able to keep the cached tree valid, at least
in theory.

^ permalink raw reply

* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Nicolas Mailhot @ 2012-02-20 19:51 UTC (permalink / raw)
  To: Jeff King; +Cc: Nicolas Mailhot, git
In-Reply-To: <20120220193006.GA30904@sigill.intra.peff.net>


Le Lun 20 février 2012 20:30, Jeff King a écrit :
> On Mon, Feb 20, 2012 at 08:24:15PM +0100, Nicolas Mailhot wrote:
>
>> > I think a good first step would be improving the error message for a
>> > 511, then. Unfortunately, it seems from the rfc draft you sent that
>> > callers are expected to parse the link out of the HTML given in the body
>> > of the response. It seems silly that there is not a Location field
>> > associated with a 511, similar to redirects.
>>
>> The URL is not lost in the HTML text, it's in the url meta field
>>
>> <meta http-equiv="refresh"
>>        content="0; url=https://login.example.net/">
>
> Sorry, but
>
>   1. That is in the HTML in the body of the response (by body I don't
>      mean the HTML <body>, but the body of the http request).
>
>   2. I don't see anything in the rfc indicating that there must be a
>      meta tag in the response. They use it in the example of the rfc,
>      but they also have human-readable text with an <a> link.  Do we yet
>      know what will be common among captive portals?
>
> You said you have a non-hypothetical case. Can you show us the response?

Not yet because it's currently non-standard custom redirection mess we're
repurposing to follow the ietf spec (got tired of being accused of running a
crap non-standard proxy by users, so now it's ll be a crap standard proxy)

The proxy response is totally configurable (a so there's no reason we won't
follow the new spec to the letter


-- 
Nicolas Mailhot

^ permalink raw reply

* Re: [PATCH 0/5] diff --ignore-case
From: Junio C Hamano @ 2012-02-20 19:47 UTC (permalink / raw)
  To: Thomas Rast; +Cc: Johannes Sixt, git, Chris Leong
In-Reply-To: <871upp4n15.fsf@thomas.inf.ethz.ch>

Thomas Rast <trast@inf.ethz.ch> writes:

> I wonder which one of us misunderstood the original request ;-)

Heh, I did ;-)

> It was
>
> } Is there any way to run diff -G with a case insensitivity flag?
>
> and I took that to mean "I want to find addition/removal of a string
> like -G does, but I don't know how it was capitalized".

I think it is just the matter of checking REG_ICASE that may be set in
revs->grep_filter.regflags, and propagating it down to the regcomp at the
beginning of diffcore_pickaxe_grep().

Want to try and see how well it works?

^ permalink raw reply

* Re: git-subtree Ready #2
From: David A. Greene @ 2012-02-20 19:34 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Avery Pennarun
In-Reply-To: <87ty2ro1zf.fsf@smith.obbligato.org>

greened@obbligato.org (David A. Greene) writes:

> greened@obbligato.org (David A. Greene) writes:
>
>>> But more important than the physical layout is the maintenance plan
>>> going forward.  Is Avery going to keep maintaining git-subtree, and we
>>> will just occasionally pull? Are you maintaining it? Where will patches
>>> go? To a github repo? To git@vger?
>
> I've attached Avery's response below.  The short summary is that he
> thinks maintaining it in the vger git repository is the way to go and
> that he's fine moving patches to/from GitHub as necessary.

So what's the next step?  I guess one of the git maintaners will have to
do a pull and merge.  Anything I need to do on this end for that to
happen?

Thanks!

                             -Dave

^ permalink raw reply

* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Jeff King @ 2012-02-20 19:30 UTC (permalink / raw)
  To: Nicolas Mailhot; +Cc: git
In-Reply-To: <72fbd4155349723da1c3c503c1c9c620.squirrel@arekh.dyndns.org>

On Mon, Feb 20, 2012 at 08:24:15PM +0100, Nicolas Mailhot wrote:

> > I think a good first step would be improving the error message for a
> > 511, then. Unfortunately, it seems from the rfc draft you sent that
> > callers are expected to parse the link out of the HTML given in the body
> > of the response. It seems silly that there is not a Location field
> > associated with a 511, similar to redirects.
> 
> The URL is not lost in the HTML text, it's in the url meta field
> 
> <meta http-equiv="refresh"
>        content="0; url=https://login.example.net/">

Sorry, but

  1. That is in the HTML in the body of the response (by body I don't
     mean the HTML <body>, but the body of the http request).

  2. I don't see anything in the rfc indicating that there must be a
     meta tag in the response. They use it in the example of the rfc,
     but they also have human-readable text with an <a> link.  Do we yet
     know what will be common among captive portals?

You said you have a non-hypothetical case. Can you show us the response?

> As for while there is no Location field, I think it's because otherwise it
> could behave like a redirect, and browser people made it plain they didn't
> want redirects of https accesses (but I wasn't there when the spec was
> written, and only skimmed the workgroup archives, so there may have been other
> reasons for this choice. I'm pretty sure it's deliberate anyway).

Even if they didn't call it Location, it would be nice to have some
machine-readable format that is understood by non-browser agents that
don't know how to parse HTML. But I recognize that is not your decision,
so don't feel obligated to defend it.

-Peff

^ permalink raw reply

* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Nicolas Mailhot @ 2012-02-20 19:24 UTC (permalink / raw)
  To: Jeff King; +Cc: Nicolas Mailhot, git
In-Reply-To: <20120220191500.GA29228@sigill.intra.peff.net>


Le Lun 20 février 2012 20:15, Jeff King a écrit :
> On Mon, Feb 20, 2012 at 07:27:08PM +0100, Nicolas Mailhot wrote:
>
>> Step 3 is a quite less obvious on a corporate network, where Internet access
>> is gated by a filtering proxy, that will let some sites pass transparently
>> but
>> require credentials to let you access others. Worst case, there are several
>> load-balanced gateways on different physical sites (to avoid spofs in case
>> of
>> planes falling on the wrong place), that do not share authentication
>> (because
>> propagating auth across physical sites is hard). So no, just launching a
>> browser is not sufficient to find the captive portal, you need to actually
>> access the URL returned by error 511 in meta information. Git should at
>> minimum report this URL.
>>
>> (and no this is not an hypothetical scenario and yes there are git users
>> trying to pass the gateways there)
>
> This is exactly the sort of information I wanted to get from a
> real-world scenario. From your initial messages, it sounded like a
> purely hypothetical thing.
>
> I think a good first step would be improving the error message for a
> 511, then. Unfortunately, it seems from the rfc draft you sent that
> callers are expected to parse the link out of the HTML given in the body
> of the response. It seems silly that there is not a Location field
> associated with a 511, similar to redirects.

The URL is not lost in the HTML text, it's in the url meta field

<meta http-equiv="refresh"
       content="0; url=https://login.example.net/">

As for while there is no Location field, I think it's because otherwise it
could behave like a redirect, and browser people made it plain they didn't
want redirects of https accesses (but I wasn't there when the spec was
written, and only skimmed the workgroup archives, so there may have been other
reasons for this choice. I'm pretty sure it's deliberate anyway).

Regards,

-- 
Nicolas Mailhot

^ permalink raw reply

* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Daniel Stenberg @ 2012-02-20 19:06 UTC (permalink / raw)
  To: Jeff King; +Cc: Nicolas Mailhot, git
In-Reply-To: <20120220154452.GA27456@sigill.intra.peff.net>

On Mon, 20 Feb 2012, Jeff King wrote:

>  3. Open a browser and say "Ah, I see. A captive portal".
>
> We should already be doing that. Adding more support could make step 3 a 
> little nicer, but like I said, I'd be more interested in seeing a real case 
> first. It may even be a feature that would be more appropriate to curl 
> (which git builds on for http access).

We're already discussing the 511 in the curl camp as well, but with even more 
sighs and hands in the air. 511 is clearly intended for HTML-understanding 
user agents and curl is not one of those. IMHO, curl will remain to simply 
help users to figure out that it is 511 and leave it at that.

As a git user, I would probably be very surprised if using 'git' suddenly 
caused by browser to pop up a captive portal login. I would prefer git to 
instead properly explain to me that is being the victim of a 511 and what I 
should do to fix it.

-- 

  / daniel.haxx.se

^ permalink raw reply

* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Jeff King @ 2012-02-20 19:15 UTC (permalink / raw)
  To: Nicolas Mailhot; +Cc: git
In-Reply-To: <cb81840f853a1d43a7da03ea24c86445.squirrel@arekh.dyndns.org>

On Mon, Feb 20, 2012 at 07:27:08PM +0100, Nicolas Mailhot wrote:

> Step 3 is a quite less obvious on a corporate network, where Internet access
> is gated by a filtering proxy, that will let some sites pass transparently but
> require credentials to let you access others. Worst case, there are several
> load-balanced gateways on different physical sites (to avoid spofs in case of
> planes falling on the wrong place), that do not share authentication (because
> propagating auth across physical sites is hard). So no, just launching a
> browser is not sufficient to find the captive portal, you need to actually
> access the URL returned by error 511 in meta information. Git should at
> minimum report this URL.
> 
> (and no this is not an hypothetical scenario and yes there are git users
> trying to pass the gateways there)

This is exactly the sort of information I wanted to get from a
real-world scenario. From your initial messages, it sounded like a
purely hypothetical thing.

I think a good first step would be improving the error message for a
511, then. Unfortunately, it seems from the rfc draft you sent that
callers are expected to parse the link out of the HTML given in the body
of the response. It seems silly that there is not a Location field
associated with a 511, similar to redirects.

-Peff

^ permalink raw reply

* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Jeff King @ 2012-02-20 19:09 UTC (permalink / raw)
  To: Daniel Stenberg; +Cc: Nicolas Mailhot, git
In-Reply-To: <alpine.DEB.2.00.1202202002330.28090@tvnag.unkk.fr>

On Mon, Feb 20, 2012 at 08:06:46PM +0100, Daniel Stenberg wrote:

> > 3. Open a browser and say "Ah, I see. A captive portal".
> >
> >We should already be doing that. Adding more support could make
> >step 3 a little nicer, but like I said, I'd be more interested in
> >seeing a real case first. It may even be a feature that would be
> >more appropriate to curl (which git builds on for http access).
> 
> We're already discussing the 511 in the curl camp as well, but with
> even more sighs and hands in the air. 511 is clearly intended for
> HTML-understanding user agents and curl is not one of those. IMHO,
> curl will remain to simply help users to figure out that it is 511
> and leave it at that.

Thanks for the input. It sounds like our best bet is to just report the
URL from a 511 better, then. Do you have any idea yet how that
information will be available to curl library users?

> As a git user, I would probably be very surprised if using 'git'
> suddenly caused by browser to pop up a captive portal login. I would
> prefer git to instead properly explain to me that is being the victim
> of a 511 and what I should do to fix it.

I agree. Even if the "step 3" in my list is "then the user starts a
browser given the URL from git's error message", that is a huge
improvement over the current state. And it retains the principle of
least surprise.

-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