* Re: [RFC PATCH] commit: Warn about encodings unsupported by iconv.
From: Alexander Gavrilov @ 2008-10-21 10:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Paul Mackerras
In-Reply-To: <7vmygy233r.fsf@gitster.siamese.dyndns.org>
On Tue, Oct 21, 2008 at 4:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Alexander Gavrilov <angavrilov@gmail.com> writes:
>
>> diff --git a/builtin-commit-tree.c b/builtin-commit-tree.c
>> index 0453425..7f325a3 100644
>> --- a/builtin-commit-tree.c
>> +++ b/builtin-commit-tree.c
>> @@ -45,6 +45,28 @@ static const char commit_utf8_warn[] =
>> "You may want to amend it after fixing the message, or set the config\n"
>> "variable i18n.commitencoding to the encoding your project uses.\n";
>>
>> +static const char commit_bad_encoding_warn[] =
>> +"Warning: commit encoding '%s' is not supported.\n"
>> +"You may want to change the value of the i18n.commitencoding config\n"
>> +"variable, and redo the commit. Use 'iconv --list' to see the list of\n"
>> +"available encoding names.\n";
>> +
>> +static void verify_commit_encoding(const char *text, const char *encoding)
>> +{
>> + if (is_encoding_utf8(encoding)) {
>> + if (!is_utf8(text))
>> + fprintf(stderr, commit_utf8_warn);
>> + }
>> +#ifndef NO_ICONV
>> + else {
>> + char *conv = reencode_string("", "utf-8", encoding);
>> + if (!conv)
>> + fprintf(stderr, commit_bad_encoding_warn, encoding);
>> + free(conv);
>> + }
>> +#endif
>> +}
>> +
> It just marks it with the encoding header. Wouldn't that mean that it
> should be possible for you to create a commit with its message encoded in
> KOI-8, and mark the resulting commit as encoded as such, on a host that is
> incapable of actually transcoding from KOI-8 to utf-8? IOW, your being
> able to encode from i18n.commitencoding to utf-8 does not have much to do
> with the validity of the configuration variable.
It may be possible to check for reencode_string("", encoding,
encoding). IConv should be able to do the identity conversion for any
valid encoding. I checked with the iconv command-line executable on my
system, and it correctly errors out on invalid names, and agrees to do
the conversion for valid ones.
> It would clarify the issues to think about what this new code would do on
> a host without iconv, if you do not have the above #ifndef/#endif pair.
> The replacement reencode_string() implementation always returns NULL, so
> the code will always warn.
>
> I am guessing that the reason you added #ifndef/#endif is because what the
> warning message says is incorrect.
>
> * "is not supported" is not correct. "is not supported HERE" may be.
>
> * "is not supported" (nor "is not supported HERE") does not matter. It
> is log-reading side that does the re-encoding, not the commit
> generating side.
The trouble is, by the time it gets to the "log-reading" side, it may
be too late to do anything, because the commits have already been
created, hashed and pushed out somewhere. And the originating side
won't notice because they would actually expect to get the messages
without conversion from their git-log. It may be amended somewhat if
gitk insists on receiving all data from git-log in utf-8 unless
explicitly told otherwise; on the other hand it may make people think
that the problem is in gitk...
> * what you would really want to say is "might be incorrectly spelled",
> but your problem is that you do not have a direct way to check that.
>
> Another reason of your "#ifndef/#endif" would be that there is no way to
> squelch the warning message after seeing it on a NO_ICONV platform.
>
> But that suggests that the "#ifndef/#endif" is not a good way to squelch
> the message. What would you do, after seeing the warning message and
> examining the situation, you know KOI-8 is a valid encoding name, your
> editor is recording what you type in the commit log message in KOI-8, you
> know you set i18n.commitencoding to KOI-8, and you know somehow your
> system is incapable of reencode_string("", "utf-8" "KOI-8")? There is no
> way to squelch the message.
A bit of a problem is that unless git implements its own encoding
alias table, the only authority on valid encoding names is iconv. So
checking would still involve running reencode_string, or looking at
iconv --list, possibly on another machine.
> So perhaps you would need some "state" variable that says "I know this
> i18n.commitencoding configuration is valid" if you go this route? But the
> reason for "I know" would be either (1) because we earlier tried
> reencode_string() and it resulted in an Ok return, or (2) because the user
> verified that the configuration is valid, even though on this particular
> system it cannot be encoded to utf-8. IOW, the latter one would be
> "because the user tells us so" --- which would be the same as trusting
> i18n.commitencoding from the beginning. I dunno.
Maybe if the option is called something like i18n.brokenIconv, people
would understand that it should not be enabled in normal
circumstances? It can also be used to tell GUI tools that git-log
cannot be trusted to do the conversion properly -- perhaps being
enabled system-wide by make install if NO_ICONV is set.
> I actually have an alternative approach to suggest.
>
> How about adding a new i18n.commit-reencode-logmessage option (boolean),
> and when it is set, we actually re-encode from i18n.commitencoding to
> "utf-8" before creating the commit object (and obviously we do not store
> "encoding" header in the resulting commit)? When the conversion fails, we
> know it failed, so the warning you added does make sense in that context.
This option changes the way data is actually processed before storing,
so it probably cannot be turned on by default -- while the check is
nearly useless _unless_ it is turned on by default.
Alexander
^ permalink raw reply
* Re: [PATCH 3/3] Add -k/--keep-going option to mergetool
From: Jeff King @ 2008-10-21 11:12 UTC (permalink / raw)
To: Charles Bailey; +Cc: git, William Pursell, Junio C Hamano, Theodore Ts'o
In-Reply-To: <1224583999-26279-3-git-send-email-charles@hashpling.org>
On Tue, Oct 21, 2008 at 11:13:19AM +0100, Charles Bailey wrote:
> This option stops git mergetool from aborting at the first failed merge.
> This allows some additional use patterns. Merge conflicts can now be
> previewed one at time and merges can also be skipped so that they can be
> performed in a later pass.
All 3 patches look good to me, and match what I expected from our
earlier discussion. But I am not too familiar with mergetool, so take my
approval with a grain of salt. :)
-Peff
^ permalink raw reply
* Re: [PATCH, RFC] diff: add option to show context between close chunks
From: Jeff King @ 2008-10-21 11:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, René Scharfe, Git Mailing List,
Davide Libenzi
In-Reply-To: <7vskqqzajy.fsf@gitster.siamese.dyndns.org>
On Tue, Oct 21, 2008 at 12:12:17AM -0700, Junio C Hamano wrote:
> Yeah. René wanted this for _human consumption_, not mechanical patch
> application, so "hardcoding" literally there in the very low level of the
> diff callchain is not quite right (it would affect format-patch which is
> primarily for mechanical application).
>
> I guess you could make the hardcoded value 1 for everybody else and 0 for
> format-patch.
I see your reasoning, but at the same time, a large portion of patches I
read are from format-patch (and René even said that he was trying to
save the user from the "apply then diff just to look at the patch"
annoyance). And I have personally, as a patch submitter, created some
format-patch output sent to the git list with -U5 to combine hunks and
make it more readable for reviewers.
Not to mention that I sometimes apply or post the output of "git diff".
To me that it implies that either:
- the increased chance of conflict is not a problem in practice, and we
should have the option on by default everywhere
- it is a problem, in which case we should ask the user to turn on the
feature manually instead of second-guessing how they will use the
resulting patch (which they might not even know, since they are
making assumptions about how other people might use the patch, and
they must decide for their situation between shipping something that
is more readable but slightly more conflict prone, or as easy to
apply as possible)
-Peff
^ permalink raw reply
* git command to read
From: Paul Mackerras @ 2008-10-21 11:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Junio,
Is there a command in git at the moment that will read commit IDs on
standard input and print the contents of each commit on standard
output (without waiting for EOF on stdin)?
I tried git rev-list --stdin --no-walk --header, but it seems to
collect all the IDs from stdin and eliminate duplicates before
outputting anything, which is not what I want. What I want is a
process that I can run from gitk where I can write IDs to its stdin
whenever gitk needs to know the contents of some commits, and know
that those contents will be turning up on the pipe from its stdout in
a timely fashion, without having to start a new process each time. Is
there a way to do that currently?
The reason I want this is to reduce gitk's memory usage. At present
it reads all the commits into memory, which takes about 160MB on the
current kernel tree just to store the contents of all the commits
(since Tcl stores strings internally as 2 bytes/character). Instead I
plan to make gitk keep a cache of commits and read in commits as
needed from an external process. When doing a search, we may need to
read in nearly all the commits, and we'll need to do it quickly.
Thanks,
Paul.
^ permalink raw reply
* Re: git command to read
From: Jeff King @ 2008-10-21 11:25 UTC (permalink / raw)
To: Paul Mackerras; +Cc: Junio C Hamano, git
In-Reply-To: <18685.47909.354146.487700@cargo.ozlabs.ibm.com>
On Tue, Oct 21, 2008 at 10:21:09PM +1100, Paul Mackerras wrote:
> Is there a command in git at the moment that will read commit IDs on
> standard input and print the contents of each commit on standard
> output (without waiting for EOF on stdin)?
How about:
git cat-file --batch
?
Unfortunately it just dumps the raw commit information instead of
allowing the usual formatting, but perhaps that is sufficient (you asked
for "contents").
-Peff
^ permalink raw reply
* Re: [PATCH 7/7] gitk: Explicitly position popup windows.
From: Paul Mackerras @ 2008-10-21 11:41 UTC (permalink / raw)
To: Alexander Gavrilov; +Cc: git
In-Reply-To: <1223449540-20457-8-git-send-email-angavrilov@gmail.com>
Alexander Gavrilov writes:
> For some reason, on Windows all transient windows are placed
> in the upper left corner of the screen. Thus, it is necessary
> to explicitly position the windows using the tk::PlaceWindow
> function.
Hmmm, this is not part of the official Tk API as far as I can see, and
having to call tk::PlaceWindow on every window we create is a bit
gross. What exactly does it do, and what effect will this change have
on Linux? Are you sure there isn't some other way to fix the problem?
Paul.
^ permalink raw reply
* Re: [PATCH 2/7] gitk: Allow forcing branch creation if it already exists.
From: Paul Mackerras @ 2008-10-21 11:38 UTC (permalink / raw)
To: Alexander Gavrilov; +Cc: git
In-Reply-To: <1223449540-20457-3-git-send-email-angavrilov@gmail.com>
Alexander Gavrilov writes:
> If gitk knows that the branch the user tries to create exists,
> it should ask whether it should overwrite it. This way the user
> can either decide to choose a new name, or move the head while
> preserving the reflog.
Applied, thanks.
Paul.
^ permalink raw reply
* Re: [PATCH 4/7] gitk: Fix file list context menu for merge commits.
From: Paul Mackerras @ 2008-10-21 11:39 UTC (permalink / raw)
To: Alexander Gavrilov; +Cc: git
In-Reply-To: <1223449540-20457-5-git-send-email-angavrilov@gmail.com>
Alexander Gavrilov writes:
> Currently it displays an ugly error box, because the
> treediffs array is not filled for such commits. This
> is clearly unacceptable.
Applied, thanks, with a better patch description, one that actually
described the remedial action. :)
Paul.
^ permalink raw reply
* Re: [PATCH 1/7] gitk: Enhance UI popup and accelerator handling.
From: Paul Mackerras @ 2008-10-21 11:35 UTC (permalink / raw)
To: Alexander Gavrilov; +Cc: git
In-Reply-To: <1223449540-20457-2-git-send-email-angavrilov@gmail.com>
Alexander Gavrilov writes:
> - Popups are supposed to be marked transient, otherwise
> the WM creates them in strange places. Besides, at
> least under kwin, transients are automatically kept
> above their parent.
> - Accelerators for menu items should be listed directly
> on the items, to make them more discoverable.
> - Some more accelerators are added, e.g. F4 for Edit View.
> - Finally, dialogs can now be accepted or dismissed using
> the Return and Escape keys.
Could you rebase this and split it into 3 patches, one each for the
transient popups, accelerators, and return/escape key bindings?
Thanks,
Paul.
^ permalink raw reply
* Re: git command to read
From: Paul Mackerras @ 2008-10-21 11:45 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20081021112521.GC17363@coredump.intra.peff.net>
Jeff King writes:
> On Tue, Oct 21, 2008 at 10:21:09PM +1100, Paul Mackerras wrote:
>
> > Is there a command in git at the moment that will read commit IDs on
> > standard input and print the contents of each commit on standard
> > output (without waiting for EOF on stdin)?
>
> How about:
>
> git cat-file --batch
>
> ?
>
> Unfortunately it just dumps the raw commit information instead of
> allowing the usual formatting, but perhaps that is sufficient (you asked
> for "contents").
Ah, thank you, that looks like just what I want. :)
Paul.
^ permalink raw reply
* Re: [PATCH 2/3] Add -n/--no-prompt option to mergetool
From: Andreas Ericsson @ 2008-10-21 11:49 UTC (permalink / raw)
To: Charles Bailey
Cc: git, Jeff King, William Pursell, Junio C Hamano,
Theodore Ts'o
In-Reply-To: <1224583999-26279-2-git-send-email-charles@hashpling.org>
Charles Bailey wrote:
> This option lets git mergetool invoke the conflict resolution program
> without waiting for a user response each time.
>
> Also added a mergetool.prompt (default true) configuration variable
> controlling the same behaviour.
>
> Signed-off-by: Charles Bailey <charles@hashpling.org>
> ---
> Documentation/config.txt | 3 +++
> Documentation/git-mergetool.txt | 11 ++++++++++-
> git-mergetool.sh | 16 +++++++++++++---
> 3 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 29369d0..b4e4ee4 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -976,6 +976,9 @@ mergetool.keepBackup::
> is set to `false` then this file is not preserved. Defaults to
> `true` (i.e. keep the backup files).
>
> +mergetool.prompt::
> + Prompt before each invocation of the merge resolution program.
> +
> pack.window::
> The size of the window used by linkgit:git-pack-objects[1] when no
> window size is given on the command line. Defaults to 10.
> diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
> index e0b2703..6d6bfe0 100644
> --- a/Documentation/git-mergetool.txt
> +++ b/Documentation/git-mergetool.txt
> @@ -7,7 +7,7 @@ git-mergetool - Run merge conflict resolution tools to resolve merge conflicts
>
> SYNOPSIS
> --------
> -'git mergetool' [--tool=<tool>] [<file>]...
> +'git mergetool' [--tool=<tool>] [-n|--no-prompt|--prompt] [<file>]...
>
> DESCRIPTION
> -----------
> @@ -60,6 +60,15 @@ variable `mergetool.<tool>.trustExitCode` can be set to `true`.
> Otherwise, 'git-mergetool' will prompt the user to indicate the
> success of the resolution after the custom tool has exited.
>
> +-n or --no-prompt::
> + Don't prompt before each invocation of the merge resolution
> + program.
> +
There is discussion already about "-n should be for dry-run!" and git's
inconsistencies in such matters. Wouldn't -y ("assume yes on prompt")
be better?
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply
* Re: git command to read
From: Alexander Gavrilov @ 2008-10-21 12:06 UTC (permalink / raw)
To: Paul Mackerras; +Cc: Jeff King, Junio C Hamano, git
In-Reply-To: <18685.49368.557513.727997@cargo.ozlabs.ibm.com>
On Tue, Oct 21, 2008 at 3:45 PM, Paul Mackerras <paulus@samba.org> wrote:
> Jeff King writes:
>
>> On Tue, Oct 21, 2008 at 10:21:09PM +1100, Paul Mackerras wrote:
>>
>> > Is there a command in git at the moment that will read commit IDs on
>> > standard input and print the contents of each commit on standard
>> > output (without waiting for EOF on stdin)?
>>
>> How about:
>>
>> git cat-file --batch
>>
>> ?
>>
>> Unfortunately it just dumps the raw commit information instead of
>> allowing the usual formatting, but perhaps that is sufficient (you asked
>> for "contents").
>
> Ah, thank you, that looks like just what I want. :)
Since cat-file does not perform any encoding conversion, you'll have
to process the encoding headers manually though.
Alexander
^ permalink raw reply
* Re: [PATCH 2/3] Add -n/--no-prompt option to mergetool
From: Charles Bailey @ 2008-10-21 12:26 UTC (permalink / raw)
To: Andreas Ericsson
Cc: git, Jeff King, William Pursell, Junio C Hamano,
Theodore Ts'o
In-Reply-To: <48FDC1CA.2080800@op5.se>
On Tue, Oct 21, 2008 at 01:49:30PM +0200, Andreas Ericsson wrote:
>
> There is discussion already about "-n should be for dry-run!" and git's
> inconsistencies in such matters. Wouldn't -y ("assume yes on prompt")
> be better?
>
I must have missed this discussion. I've just had a very quick look at
a handful of basic modifying git commands (merge, pull, commit,
checkout, reset, revert) and only found 'add' that used -n as
--dry-run.
That said, I've no real objections to -y if that makes for a better
consensus.
--
Charles Bailey
http://ccgi.hashpling.plus.com/blog/
^ permalink raw reply
* Re: [PATCH 7/7] gitk: Explicitly position popup windows.
From: Alexander Gavrilov @ 2008-10-21 12:52 UTC (permalink / raw)
To: Paul Mackerras; +Cc: git
In-Reply-To: <18685.49152.25344.398737@cargo.ozlabs.ibm.com>
On Tue, Oct 21, 2008 at 3:41 PM, Paul Mackerras <paulus@samba.org> wrote:
> Alexander Gavrilov writes:
>
>> For some reason, on Windows all transient windows are placed
>> in the upper left corner of the screen. Thus, it is necessary
>> to explicitly position the windows using the tk::PlaceWindow
>> function.
>
> Hmmm, this is not part of the official Tk API as far as I can see, and
> having to call tk::PlaceWindow on every window we create is a bit
> gross. What exactly does it do, and what effect will this change have
> on Linux? Are you sure there isn't some other way to fix the problem?
It is just a convenient helper function that can explicitly compute
and set the window position in a number of ways. It is used in Tk's
dialog implementations. If you don't like using an unofficial
function, I can pull out the relevant ~8 lines of code as a separate
function in gitk.
http://objectmix.com/tcl/390381-tile-dialog-boxes-not-transient-parent-under-windows.html
http://wiki.tcl.tk/1254
Alexander
^ permalink raw reply
* Re: [PATCH 2/2] rm: loosen safety valve for empty files
From: Jeff King @ 2008-10-21 13:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v4p361x1f.fsf@gitster.siamese.dyndns.org>
On Mon, Oct 20, 2008 at 07:50:36PM -0700, Junio C Hamano wrote:
> in addition to what you are trying to fix here with "git rm". With such a
> change, your "git rm empty" code can also distinguish between an empty
> blob the user wanted to add _as the final contents_, and a path that has
> been marked with "add -N", and behave differently (the former would not
> require -f while the latter would).
Your reasoning makes sense.
> As an interim measure, I suspect your patch is an improvement from the
> current state of affairs, but the above test will then break when an
> improvement to "git add -N" implementation outlined above materializes.
Yeah, I actually considered writing it as "git add -N", but I thought it
better that the person who changes the behavior breaks the test, reads
the commit message, and decides then what the best behavior is.
But if you are comfortable saying now "this is what the behavior
_should_ be", then I think the test modifications you proposed make
sense (and after reading your discussion above, I think the behavior you
propose is reasonable).
> So how about changing the test to explicitly check that a path that was
> added by "git add -N" can be removed, and either (1) not check about an
> empty blob that was explicitly added by the user, or (2) check that an
> empty blob that was explicitly added by the user cannot be "git rm"'ed
> without -f, with expect_failure?
Patch below. I tried to expand the commit message to explain the
situation to the future developer who extends intent-to-add, but please
mark up if anything seems unclear.
-- >8 --
rm: loosen safety valve for empty files
If a file is different between the working tree copy, the
index, and the HEAD, then we do not allow it to be deleted
without --force.
However, this is overly tight in the face of "git add
--intent-to-add":
$ git add --intent-to-add file
$ : oops, I don't actually want to stage that yet
$ git rm --cached file
error: 'empty' has staged content different from both the
file and the HEAD (use -f to force removal)
$ git rm -f --cached file
Unfortunately, there is currently no way to distinguish
between an empty file that has been added and an "intent to
add" file. The ideal behavior would be to disallow the
former while allowing the latter.
This patch loosens the safety valve to allow the deletion
only if we are deleting the cached entry and the cached
content is empty. This covers the intent-to-add situation,
and assumes there is little harm in not protecting users who
have legitimately added an empty file. In many cases,
the file will still be empty, in which case the safety valve
does not trigger anyway (since the content remains untouched
in the working tree). Otherwise, we do remove the fact that
no content was staged, but given that the content is by
definition empty, it is not terribly difficult for a user to
recreate it.
However, we still document the desired behavior in the form
of two tests. One checks the correct removal of an
intent-to-add file. The other checks that we still disallow
removal of empty files, but is marked as expect_failure to
indicate this compromise. If the intent-to-add feature is
ever extended to differentiate between normal empty files
and intent-to-add files, then the safety valve can be
re-tightened.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin-rm.c | 3 ++-
cache.h | 1 +
read-cache.c | 2 +-
t/t3600-rm.sh | 13 +++++++++++++
4 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/builtin-rm.c b/builtin-rm.c
index e06640c..b7126e3 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -79,7 +79,8 @@ static int check_local_mod(unsigned char *head, int index_only)
|| hashcmp(ce->sha1, sha1))
staged_changes = 1;
- if (local_changes && staged_changes)
+ if (local_changes && staged_changes &&
+ !(index_only && is_empty_blob_sha1(ce->sha1)))
errs = error("'%s' has staged content different "
"from both the file and the HEAD\n"
"(use -f to force removal)", name);
diff --git a/cache.h b/cache.h
index cdbeb48..b0edbf9 100644
--- a/cache.h
+++ b/cache.h
@@ -519,6 +519,7 @@ static inline void hashclr(unsigned char *hash)
{
memset(hash, 0, 20);
}
+extern int is_empty_blob_sha1(const unsigned char *sha1);
int git_mkstemp(char *path, size_t n, const char *template);
diff --git a/read-cache.c b/read-cache.c
index fdb41b8..2c45086 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -160,7 +160,7 @@ static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)
return 0;
}
-static int is_empty_blob_sha1(const unsigned char *sha1)
+int is_empty_blob_sha1(const unsigned char *sha1)
{
static const unsigned char empty_blob_sha1[20] = {
0xe6,0x9d,0xe2,0x9b,0xb2,0xd1,0xd6,0x43,0x4b,0x8b,
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 66aca99..5b4d6f7 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -187,6 +187,19 @@ test_expect_success 'but with -f it should work.' '
test_must_fail git ls-files --error-unmatch baz
'
+test_expect_failure 'refuse to remove cached empty file with modifications' '
+ touch empty &&
+ git add empty &&
+ echo content >empty &&
+ test_must_fail git rm --cached empty
+'
+
+test_expect_success 'remove intent-to-add file without --force' '
+ echo content >intent-to-add &&
+ git add -N intent-to-add
+ git rm --cached intent-to-add
+'
+
test_expect_success 'Recursive test setup' '
mkdir -p frotz &&
echo qfwfq >frotz/nitfol &&
--
1.6.0.2.808.gf6cf2
^ permalink raw reply related
* [PATCH] git-remote: list branches in vertical lists
From: Johannes Sixt @ 2008-10-21 14:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
From: Johannes Sixt <johannes.sixt@telecom.at>
Previously, branches were listed on a single line in each section. But
if there are many branches, then horizontal, line-wrapped lists are very
inconvenient to scan for a human. This makes the lists vertical, i.e one
branch per line is printed.
This does mean that users' scripts must be updated because the output
format changed, but the result is friendlier to the eye *and* easier to
parse.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
Documentation/user-manual.txt | 4 +++-
builtin-remote.c | 11 +++++------
t/t5505-remote.sh | 14 +++++++++-----
3 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index 08d1310..645d752 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -4356,7 +4356,9 @@ $ git remote show example # get details
* remote example
URL: git://example.com/project.git
Tracked remote branches
- master next ...
+ master
+ next
+ ...
$ git fetch example # update branches from example
$ git branch -r # list all remote branches
-----------------------------------------------
diff --git a/builtin-remote.c b/builtin-remote.c
index 90a4e35..1b1697b 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -416,10 +416,9 @@ static void show_list(const char *title, struct string_list *list,
return;
printf(title, list->nr > 1 ? "es" : "", extra_arg);
- printf("\n ");
- for (i = 0; i < list->nr; i++)
- printf("%s%s", i ? " " : "", list->items[i].string);
printf("\n");
+ for (i = 0; i < list->nr; i++)
+ printf(" %s\n", list->items[i].string);
}
static int get_remote_ref_states(const char *name,
@@ -515,17 +514,17 @@ static int show(int argc, const char **argv)
show_list(" Tracked remote branch%s", &states.tracked, "");
if (states.remote->push_refspec_nr) {
- printf(" Local branch%s pushed with 'git push'\n ",
+ printf(" Local branch%s pushed with 'git push'\n",
states.remote->push_refspec_nr > 1 ?
"es" : "");
for (i = 0; i < states.remote->push_refspec_nr; i++) {
struct refspec *spec = states.remote->push + i;
- printf(" %s%s%s%s", spec->force ? "+" : "",
+ printf(" %s%s%s%s\n",
+ spec->force ? "+" : "",
abbrev_branch(spec->src),
spec->dst ? ":" : "",
spec->dst ? abbrev_branch(spec->dst) : "");
}
- printf("\n");
}
/* NEEDSWORK: free remote */
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index e5137dc..e6cf2c7 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -28,7 +28,7 @@ tokens_match () {
}
check_remote_track () {
- actual=$(git remote show "$1" | sed -n -e '$p') &&
+ actual=$(git remote show "$1" | sed -e '1,/Tracked/d') &&
shift &&
tokens_match "$*" "$actual"
}
@@ -118,9 +118,11 @@ cat > test/expect << EOF
New remote branch (next fetch will store in remotes/origin)
master
Tracked remote branches
- side master
+ side
+ master
Local branches pushed with 'git push'
- master:upstream +refs/tags/lastbackup
+ master:upstream
+ +refs/tags/lastbackup
EOF
test_expect_success 'show' '
@@ -147,9 +149,11 @@ cat > test/expect << EOF
Remote branch merged with 'git pull' while on branch master
master
Tracked remote branches
- master side
+ master
+ side
Local branches pushed with 'git push'
- master:upstream +refs/tags/lastbackup
+ master:upstream
+ +refs/tags/lastbackup
EOF
test_expect_success 'show -n' '
--
1.6.0.2.1573.gdf533
^ permalink raw reply related
* Re: [PATCH] rehabilitate 'git index-pack' inside the object store
From: Nguyen Thai Ngoc Duy @ 2008-10-21 14:57 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.LFD.2.00.0810202110380.26244@xanadu.home>
On 10/21/08, Nicolas Pitre <nico@cam.org> wrote:
> Before commit d0b92a3f6e it was possible to run 'git index-pack'
> directly in the .git/objects/pack/ directory. Restore that ability.
I am sorry I did not catch this in the first place. While the fix
should be fine for "git index-pack". I wonder what can happen for
other setup_*_gently()'s consumers. Other commands may be affected too
(e.g. running "git apply" or "git bundle" from inside .git). Maybe we
should let setup_*_gently() do read-only stuff only and let its
consumers to handle cwd. I recall Jeff has plan about worktree setup
rework, though could not find the thread. Will think more of it
tomorrow.
--
Duy
^ permalink raw reply
* Re: [PATCH] rehabilitate 'git index-pack' inside the object store
From: Jeff King @ 2008-10-21 15:02 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Nicolas Pitre, Junio C Hamano, git
In-Reply-To: <fcaeb9bf0810210757w1c14e0a3x1eb61a589a089f10@mail.gmail.com>
On Tue, Oct 21, 2008 at 09:57:04PM +0700, Nguyen Thai Ngoc Duy wrote:
> (e.g. running "git apply" or "git bundle" from inside .git). Maybe we
> should let setup_*_gently() do read-only stuff only and let its
> consumers to handle cwd. I recall Jeff has plan about worktree setup
> rework, though could not find the thread. Will think more of it
> tomorrow.
If by "plan" you mean "intense desire to see it fixed", then yes, I have
a plan. But there isn't a concrete plan yet, and I'm not sure when I'll
get to it. I have a vague assumption that we should move in the
direction of just setting up as much of the environment as possible
(finding git dir, finding work tree, reading config, etc) as soon as
possible, but not producing any error messages or dying as a result.
Then those commands which want to enforce that some setup has occurred
can easily do so.
-Peff
^ permalink raw reply
* Re: Working with remotes; cloning remote references
From: Marc Branchaud @ 2008-10-21 15:17 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Peter Harris, git
In-Reply-To: <48FDA5A0.8030506@drmicha.warpmail.net>
Michael J Gruber wrote:
>
>> clone/$ git config remote.origin.fetch \
>> '+refs/remotes/ThingOne/*:refs/remotes/ThingOne/*'
>
> If you want to fetch main's local branches also, use option "--add" here
> so that you don't override the default fetch refspec (forgot last time,
> sorry).
Okay, got it.
> I thought you wanted to avoid pulling directly from ThingOne to clone?
Ah, I see (part of) our disconnect here -- sorry for not explaining it
clearly before!
Yes, indeed, I do want to pull directly from ThingOne into the clone.
The scenario is that there's a bunch of us sharing the main repo, and
when some upstream changes happen in ThingOne I'd like for one of us to
be able to clone the main repo, pull the changes from ThingOne into the
clone (fixing any conflicts; remember that we have our own changes in
ThingOne's code), then push the merged changes back to main for everyone
else to grab.
> If you pull directly you might as well set up the same remote config as
> on main: for the correct pull line you need to know the same info as for
> the correct remote config.
Yes, I see that -- it's why I'm basically satisfied with being able to do
clone/$ git pull -s subtree /path/to/ThingOne master
from the clone, and all I'm doing now is kvetching about having to
remember the location and branch of ThingOne when that was already
configured in main (and thanks for being patient with my kvetching!).
> git fetch
> git merge -s subtree remotes/ThingOne/master
>
> should do the trick.
AFAICT that only refers to the clone's local branch for the ThingOne
repo. The above git-config command doesn't add the ThingOne repo's URL
to the clone, so I'm still stuck having to use that URL directly to pull
changes from ThingOne into the clone. (And when I use the URL directly,
I have to use a branch name that's defined in the ThingOne repo. I'd
like to also be able to use whatever branch name got set up in main when
"git remote add" was run.)
> If that works you can set up things so that pulling
> from origin (pulling when you're in your integration branch) does that
> merge automatically, using branch.integrationbranch.remote=origin,
> branch.integrationbranch.merge=remotes/ThingOne/master (untested ;) ).
>
> To be clear: The idea here is that main decides which ThingOne branch to
> store in remotes/ThingOne/master and where to get it from; clones always
> pull that one.
I think that "where to get it from" part is what I'm going on about.
There doesn't seem to be a way for the main repo to tell the clone where
the ThingOne repo is, so that the clone can pull in ThingOne changes
directly.
>> I just feel that there are some
>> situations where you want the origin's remotes in your clone, and some
>> where you don't, and git should let you decide.
>
> Well, it let's you decide: It tracks local branches by default, and
> using additional "git config" you can track remotes as well. You can
> also use the "--mirror" option to "git clone" or "git remote add", but
> that has other side effects.
I think we're mis-communicating, mainly because I'm not yet able to
express things well in git-speak. Let me give it another stab...
I believe git lets you track the origin's _branches_ not the origin's
_remotes_. I don't think --mirror does what I'm looking for, because
(side effects aside) it only deals with branches, not remotes.
I find myself getting confused, and I think it's because the files in
.git/refs/remotes/ are indeed tracking branches on remote repositories.
So I think our conversation gets a bit circular because our ideas of a
"remote" differ.
"git remote add" does two things (maybe more?): It adds a [remote]
section to the .git/config file, and it adds a branch reference in
.git/refs/remotes/. I think what I'd like is for the clone to be able
to obtain both these things from the origin. The reason I think it's
useful is that it would let the clone pull directly from the origin's
remote repositories, without having to directly specify the remote
repository's URL and branch name.
Fundamentally, I'm looking to do exactly
clone/$ git pull -s subtree /path/to/ThingOne master
i.e. pull stuff from one of main's remotes directly into the clone. But
I want to replace the "/path/to/ThingOne master" part with something
that means "use whatever URL and branch name was defined in the origin
for this remote".
My questions are: Am I right in thinking this is desirable? Is there
already some way to do this? If not, is it something worth
implementing? (I'm happy to roll up my own sleeves here...)
I hope that clarifies things. Sorry for taking so long to get here!
Marc
^ permalink raw reply
* Re: [PATCH] rehabilitate 'git index-pack' inside the object store
From: Nicolas Pitre @ 2008-10-21 15:54 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, git
In-Reply-To: <fcaeb9bf0810210757w1c14e0a3x1eb61a589a089f10@mail.gmail.com>
On Tue, 21 Oct 2008, Nguyen Thai Ngoc Duy wrote:
> On 10/21/08, Nicolas Pitre <nico@cam.org> wrote:
> > Before commit d0b92a3f6e it was possible to run 'git index-pack'
> > directly in the .git/objects/pack/ directory. Restore that ability.
>
> I am sorry I did not catch this in the first place. While the fix
> should be fine for "git index-pack". I wonder what can happen for
> other setup_*_gently()'s consumers. Other commands may be affected too
> (e.g. running "git apply" or "git bundle" from inside .git).
Normally you should not be running such commands inside the .git
directory. The index-pack command is a bit special in that regard.
Nicolas
^ permalink raw reply
* Re: [PATCH] Teach/Fix pull/fetch -q/-v options
From: Tuncer Ayaz @ 2008-10-21 16:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vwsg22568.fsf@gitster.siamese.dyndns.org>
On Tue, Oct 21, 2008 at 1:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:
>
>> On Sun, Oct 19, 2008 at 11:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>>> @@ -23,6 +24,10 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase)
>>>> while :
>>>> do
>>>> case "$1" in
>>>> + -q|--quiet)
>>>> + verbosity="$verbosity -q" ;;
>>>> + -v|--verbose)
>>>> + verbosity="$verbosity -v" ;;
>>>
>>> You know verbosity flags (-q and -v) are "the last one wins", so I do not
>>> see much point in this concatenation.
>>
>> Without concatenation I would need to analyze the content
>> of the variable each time the option is passed to the shell
>> script. Do you know of a simpler/better way still keeping the
>> functionality that
>> $ git pull -q -v --quiet --verbose --quiet gives verbosity=QUIET
>> and
>> $ git pull -q -v --quiet --verbose --quiet -v yields verbosity=VERBOSE
>> ?
>
> Wouldn't
>
> verbosity=
> while :
> do
> case "$1" in
> -q|--quiet) verbosity=-q ;;
> -v|--verbose) verbosity=-v ;;
> ... others ...
> esac
> shift
> done
> git pull $verbosity other options
>
> give the -q for the former and -v for the latter to "git pull"?
Yes that is much simpler and works :). Thanks.
Please see my next patch in a few minutes.
I might not reply before the weekend as I'm pretty busy, btw.
^ permalink raw reply
* [PATCH] Teach/Fix pull/fetch -q/-v options
From: Tuncer Ayaz @ 2008-10-21 16:30 UTC (permalink / raw)
To: git; +Cc: gitster
After fixing clone -q I noticed that pull -q does not do what
it's supposed to do and implemented --quiet/--verbose by
adding it to builtin-merge and fixing two places in builtin-fetch.
I have not touched/adjusted contrib/completion/git-completion.bash
but can take a look if wanted. I think it already needs one or two
adjustments caused by recent --OPTIONS changes in master.
I've tested the following invocations with the below changes applied:
$ git pull
$ git pull -q
$ git pull -v
$ git fetch -q
$ git pull -q -v -q
Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>
---
Documentation/merge-options.txt | 8 ++++++++
builtin-fetch.c | 21 +++++++++++----------
builtin-merge.c | 22 +++++++++++++++-------
git-pull.sh | 11 ++++++++---
4 files changed, 42 insertions(+), 20 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 007909a..427cdef 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -1,3 +1,11 @@
+-q::
+--quiet::
+ Operate quietly.
+
+-v::
+--verbose::
+ Be verbose.
+
--stat::
Show a diffstat at the end of the merge. The diffstat is also
controlled by the configuration option merge.stat.
diff --git a/builtin-fetch.c b/builtin-fetch.c
index ee93d3a..b067512 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -22,16 +22,17 @@ enum {
TAGS_SET = 2
};
-static int append, force, keep, update_head_ok, verbose, quiet;
+static int append, force, keep, update_head_ok;
static int tags = TAGS_DEFAULT;
static const char *depth;
static const char *upload_pack;
static struct strbuf default_rla = STRBUF_INIT;
static struct transport *transport;
+static enum { QUIET, NORMAL, VERBOSE } verbosity = NORMAL;
static struct option builtin_fetch_options[] = {
- OPT__QUIET(&quiet),
- OPT__VERBOSE(&verbose),
+ OPT_SET_INT('v', "verbose", &verbosity, "be verbose", VERBOSE),
+ OPT_SET_INT('q', "quiet", &verbosity, "operate quietly", QUIET),
OPT_BOOLEAN('a', "append", &append,
"append to .git/FETCH_HEAD instead of overwriting"),
OPT_STRING(0, "upload-pack", &upload_pack, "PATH",
@@ -192,7 +193,6 @@ static int s_update_ref(const char *action,
static int update_local_ref(struct ref *ref,
const char *remote,
- int verbose,
char *display)
{
struct commit *current = NULL, *updated;
@@ -210,7 +210,7 @@ static int update_local_ref(struct ref *ref,
die("object %s not found", sha1_to_hex(ref->new_sha1));
if (!hashcmp(ref->old_sha1, ref->new_sha1)) {
- if (verbose)
+ if (verbosity == VERBOSE)
sprintf(display, "= %-*s %-*s -> %s", SUMMARY_WIDTH,
"[up to date]", REFCOL_WIDTH, remote,
pretty_ref);
@@ -366,18 +366,19 @@ static int store_updated_refs(const char *url, const char *remote_name,
note);
if (ref)
- rc |= update_local_ref(ref, what, verbose, note);
+ rc |= update_local_ref(ref, what, note);
else
sprintf(note, "* %-*s %-*s -> FETCH_HEAD",
SUMMARY_WIDTH, *kind ? kind : "branch",
REFCOL_WIDTH, *what ? what : "HEAD");
if (*note) {
- if (!shown_url) {
+ if (verbosity > QUIET && !shown_url) {
fprintf(stderr, "From %.*s\n",
url_len, url);
shown_url = 1;
}
- fprintf(stderr, " %s\n", note);
+ if (verbosity > QUIET)
+ fprintf(stderr, " %s\n", note);
}
}
fclose(fp);
@@ -622,9 +623,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
remote = remote_get(argv[0]);
transport = transport_get(remote, remote->url[0]);
- if (verbose >= 2)
+ if (verbosity == VERBOSE)
transport->verbose = 1;
- if (quiet)
+ if (verbosity == QUIET)
transport->verbose = -1;
if (upload_pack)
set_option(TRANS_OPT_UPLOADPACK, upload_pack);
diff --git a/builtin-merge.c b/builtin-merge.c
index 5e7910b..76e2890 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -50,6 +50,7 @@ static unsigned char head[20], stash[20];
static struct strategy **use_strategies;
static size_t use_strategies_nr, use_strategies_alloc;
static const char *branch;
+static enum { QUIET, NORMAL, VERBOSE } verbosity = NORMAL;
static struct strategy all_strategy[] = {
{ "recursive", DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -152,6 +153,8 @@ static int option_parse_n(const struct option *opt,
}
static struct option builtin_merge_options[] = {
+ OPT_SET_INT('v', "verbose", &verbosity, "be verbose", VERBOSE),
+ OPT_SET_INT('q', "quiet", &verbosity, "operate quietly", QUIET),
{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
"do not show a diffstat at the end of the merge",
PARSE_OPT_NOARG, option_parse_n },
@@ -250,7 +253,8 @@ static void restore_state(void)
/* This is called when no merge was necessary. */
static void finish_up_to_date(const char *msg)
{
- printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
+ if (verbosity > QUIET)
+ printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
drop_save();
}
@@ -331,14 +335,15 @@ static void finish(const unsigned char *new_head, const char *msg)
if (!msg)
strbuf_addstr(&reflog_message, getenv("GIT_REFLOG_ACTION"));
else {
- printf("%s\n", msg);
+ if (verbosity > QUIET)
+ printf("%s\n", msg);
strbuf_addf(&reflog_message, "%s: %s",
getenv("GIT_REFLOG_ACTION"), msg);
}
if (squash) {
squash_message();
} else {
- if (!merge_msg.len)
+ if (verbosity > QUIET && !merge_msg.len)
printf("No merge message -- not updating HEAD\n");
else {
const char *argv_gc_auto[] = { "gc", "--auto", NULL };
@@ -872,6 +877,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, builtin_merge_options,
builtin_merge_usage, 0);
+ if (verbosity > QUIET)
+ show_diffstat = 0;
if (squash) {
if (!allow_fast_forward)
@@ -1013,10 +1020,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
strcpy(hex, find_unique_abbrev(head, DEFAULT_ABBREV));
- printf("Updating %s..%s\n",
- hex,
- find_unique_abbrev(remoteheads->item->object.sha1,
- DEFAULT_ABBREV));
+ if (verbosity > QUIET)
+ printf("Updating %s..%s\n",
+ hex,
+ find_unique_abbrev(remoteheads->item->object.sha1,
+ DEFAULT_ABBREV));
strbuf_addstr(&msg, "Fast forward");
if (have_message)
strbuf_addstr(&msg,
diff --git a/git-pull.sh b/git-pull.sh
index 75c3610..c982d08 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -16,13 +16,17 @@ cd_to_toplevel
test -z "$(git ls-files -u)" ||
die "You are in the middle of a conflicted merge."
-strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
+strategy_args= no_stat= no_commit= squash= no_ff= log_arg= verbosity=
curr_branch=$(git symbolic-ref -q HEAD)
curr_branch_short=$(echo "$curr_branch" | sed "s|refs/heads/||")
rebase=$(git config --bool branch.$curr_branch_short.rebase)
while :
do
case "$1" in
+ -q|--quiet)
+ verbosity=-q ;;
+ -v|--verbose)
+ verbosity=-v ;;
-n|--no-stat|--no-summary)
no_stat=-n ;;
--stat|--summary)
@@ -121,7 +125,7 @@ test true = "$rebase" && {
"refs/remotes/$origin/$reflist" 2>/dev/null)"
}
orig_head=$(git rev-parse --verify HEAD 2>/dev/null)
-git fetch --update-head-ok "$@" || exit 1
+git fetch $verbosity --update-head-ok "$@" || exit 1
curr_head=$(git rev-parse --verify HEAD 2>/dev/null)
if test "$curr_head" != "$orig_head"
@@ -181,5 +185,6 @@ merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
test true = "$rebase" &&
exec git-rebase $strategy_args --onto $merge_head \
${oldremoteref:-$merge_head}
-exec git-merge $no_stat $no_commit $squash $no_ff $log_arg $strategy_args \
+exec git-merge $verbosity $no_stat $no_commit \
+ $squash $no_ff $log_arg $strategy_args \
"$merge_name" HEAD $merge_head
--
1.6.0.2.GIT
^ permalink raw reply related
* Re: [PATCH] git-remote: list branches in vertical lists
From: Johannes Schindelin @ 2008-10-21 16:49 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <48FDEA82.5050903@viscovery.net>
Hi,
On Tue, 21 Oct 2008, Johannes Sixt wrote:
> Previously, branches were listed on a single line in each section. But
> if there are many branches, then horizontal, line-wrapped lists are very
> inconvenient to scan for a human. This makes the lists vertical, i.e one
> branch per line is printed.
>
> This does mean that users' scripts must be updated because the output
> format changed, but the result is friendlier to the eye *and* easier to
> parse.
My initial reaction to that was: add an option, and keep the old behavior
then.
But on second thought: No script has any business scanning the output of
git-remote. That command is a pure convenience wrapper, and scripts
trying to list remote branches should use git show-ref instead.
So I'd say: replace the last comment with
Since "git remote" is porcelain, we can easily make this
backwards-incompatible change.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATHv2 6/8] gitweb: retrieve snapshot format from PATH_INFO
From: Jakub Narebski @ 2008-10-21 16:44 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <1224426270-27755-1-git-send-email-giuseppe.bilotta@gmail.com>
I like the idea behind this patch, to enable to use path_info for as
much gitweb parameters as possible. After this patch series the only
parameters which wouldn't be possible to represent in path_info would
be:
* @extra_options ('opt') multi-valued parameter, used to pass
thinks like '--no-merges', which cannot be fit in the "simplified"
list-like (as opposed to hash-like query string) path_info URL.
* $searchtype ('st') and $searchtext ('s') etc. parameters, which
are generated by HTML form, and are naturally generated in query
string format.
* $page ('pg') parameter, which could theoretically be added as last
part of path_info URL, for example $project/next/2/... if not for
pesky $project/history/next:/Documentation/2/ where you cannot be
sure that having /<number>/ at the end is rare.
* $order ('o') parameter, which would be hard to fit in path_info,
with its limitation of parameters being specified by position.
Or even next to impossible.
* 'by_tag'...
But I'd rather have this patch series to be in separate thread...
On Sun, 19 Oct 2008, Giuseppe Bilotta wrote:
> We parse requests for $project/snapshot/$head.$sfx as equivalent to
> $project/snapshot/$head?sf=$sfx, where $sfx is any of the known
> (although not necessarily supported) snapshot formats (or its default
> suffix).
>
> The filename for the resulting package preserves the requested
> extensions (so asking for a .tgz gives a .tgz, and asking for a .tar.gz
> gives a .tar.gz), although for obvious reasons it doesn't preserve the
> basename (git/snapshot/next.tgz returns a file names git-next.tgz).
That is a bit of difference from sf=<format> in CGI query string, where
<format> is always a name of a format (for example 'tgz' or 'tbz2'),
and actual suffix is defined in %known_snapshot_formats (for example
'.tar.gz' and '.tar.bz2' respectively). Now you can specify snapshot
format either either by its name, for example 'tgz' (which is simple
lookup in hash) which result in proposed filename with '.tgz' suffix,
or you can specify suffix, for example 'tar.gz' (which requires
searching through all hash) which result in proposed filename with
'.tar.gz' suffix.
This is a bit of inconsistency; to be consistent with how we handle
'sf' CGI parameter we would translate 'tgz' $sfx into 'tar.gz' in
snapshot filename. This would also cover currently purely theoretical
case when different snapshot formats (for example 'tgz' and 'tgz9')
would use the same snapshot suffix (extension), but differ for example
in parameters passed to compressor (for example '-9' or '--best' in
the 'tgz9' case).
On the other hand one would expect that when URL which looks like
URL to snapshot ends with '.$sfx', then filename for snapshot would
also end with '.$sfx'.
This certainly requires some further thoughts.
>
> This introduces a potential case for ambiguity if a project has a head
> that ends with a snapshot-like suffix (.zip, .tgz, .tar.gz, etc) and the
> sf CGI parameter is not present; however, gitweb only produces URLs with
> the sf parameter, so this is only a potential issue for hand-coded URLs
> for extremely unusual project.
I think you wanted to say here "_currently_ produces URLs with the 'sf'
parameter" as the next patch in series changes this.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>
> I had second thoughts on this. Now we always look for the snapshot extension if
> the sf CGI parameter is missing, even if the project has a head that matches
> the full pseudo-refname $head.$sfx.
>
> The reason for this is that (1) there is no ambiguity for gitweb-generated
> URLs (2) the only URLs that could fail are hand-made URLs for extremely
> unusual projects and (3) it allows us to set gitweb up to generate
> (unambiguous) URLs without the sf CGI parameter.
This is also simpler and cheaper solution.
>
> This also means that I can add 3 patches to the series, instead of just one:
> * patch #6 that parses the new format
> * patch #7 that generates the new URLs
> * patch #8 for some code refactoring
Now, I haven't yet read the last patch in series, so I don't know if
it is independent refactoring, making sense even before patches named
#6 and #7 here, or is it connected with searching for snapshot format
by suffix it uses. If the former, it should be done upfront, as it
shouldn't need discussion, and being easier to be accepted into git.git.
If the latter, then it should probably be folded (squashed) into #6,
first patch in the series.
>
> gitweb/gitweb.perl | 34 ++++++++++++++++++++++++++++++++++
> 1 files changed, 34 insertions(+), 0 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 99c8c20..e9e9e60 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -609,6 +609,40 @@ sub evaluate_path_info {
> $input_params{'hash_parent'} ||= $parentrefname;
> }
> }
> +
> + # for the snapshot action, we allow URLs in the form
> + # $project/snapshot/$hash.ext
> + # where .ext determines the snapshot and gets removed from the
> + # passed $refname to provide the $hash.
> + #
> + # To be able to tell that $refname includes the format extension, we
> + # require the following two conditions to be satisfied:
> + # - the hash input parameter MUST have been set from the $refname part
> + # of the URL (i.e. they must be equal)
This means no "$project/.tgz?h=next", isn't it?
> + # - the snapshot format MUST NOT have been defined already
I would add "which means that 'sf' parameter is not set in URL", or
something like that as the last line of above comment.
I like that the code is so well commented, by the way.
> + if ($input_params{'action'} eq 'snapshot' && defined $refname &&
> + $refname eq $input_params{'hash'} &&
Minor nit.
I would use here (the question of style / better readability):
+ if ($input_params{'action'} eq 'snapshot' &&
+ defined $refname && $refname eq $input_params{'hash'} &&
to have both conditions about $refname in the same line.
> + !defined $input_params{'snapshot_format'}) {
> + # We loop over the known snapshot formats, checking for
> + # extensions. Allowed extensions are both the defined suffix
> + # (which includes the initial dot already) and the snapshot
> + # format key itself, with a prepended dot
> + while (my ($fmt, %opt) = each %known_snapshot_formats) {
> + my $hash = $refname;
> + my $sfx;
> + $hash =~ s/(\Q$opt{'suffix'}\E|\Q.$fmt\E)$//;
> + next unless $sfx = $1;
> + # a valid suffix was found, so set the snapshot format
> + # and reset the hash parameter
> + $input_params{'snapshot_format'} = $fmt;
> + $input_params{'hash'} = $hash;
> + # we also set the format suffix to the one requested
> + # in the URL: this way a request for e.g. .tgz returns
> + # a .tgz instead of a .tar.gz
> + $known_snapshot_formats{$fmt}{'suffix'} = $sfx;
> + last;
> + }
I'm not sure if it worth (see comment at the beginning of this mail)
adding this code, or just allow $sfx to be snapshot _name_ (key in
%known_snapshot_formats hash).
Otherwise it would be as simple as checking if $known_snapshot_formats{$sfx}
exists (assuming that snapshot format names does not contain '.').
If we decide to go more complicated route, then refactoring it in such
a way that suffixes are also keys to %known_snapshot_formats would be
preferred... err, sorry, not so simple. But refactoring this check
into separate subroutine (as I think last patch in series does) would
be good idea.
Also, I'd rather you checked if the $refname part contains '.' for it
to even consider that it can be suffix.
> + }
> }
> evaluate_path_info();
>
> --
> 1.5.6.5
>
>
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH] rehabilitate 'git index-pack' inside the object store
From: Johannes Schindelin @ 2008-10-21 17:02 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Nicolas Pitre, Junio C Hamano, git
In-Reply-To: <fcaeb9bf0810210757w1c14e0a3x1eb61a589a089f10@mail.gmail.com>
Hi,
On Tue, 21 Oct 2008, Nguyen Thai Ngoc Duy wrote:
> Maybe we should let setup_*_gently() do read-only stuff only and let its
> consumers to handle cwd.
I think that makes sense. This would allow setup*gently() to be much
cleaner, and only setup_git_directory() itself would do the
chdir(worktree).
However, let's think this really through this time, so that that darned
worktree stuff is fixed for good.
So I propose this change in semantics:
- setup_git_directory_gently(): rename to discover_git_directory(),
and avoid any chdir() at all.
- setup_git_directory(): keep the semantics that it chdir()s to the
worktree, or to the git directory for bare repositories.
Using _gently() even for RUN_SETUP builtins should solve the long standing
pager problem, too.
Ciao,
Dscho
^ 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