* [PATCH v2 0/3] count-objects improvements
From: Nguyễn Thái Ngọc Duy @ 2013-02-08 3:48 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1359982145-10792-1-git-send-email-pclouds@gmail.com>
This series:
- updates count-objects -v documentation, describe each line in detail
- counts garbage files in pack directory in addition to loose odb
- shows how much disk space consumed by garbage files
Nguyễn Thái Ngọc Duy (3):
git-count-objects.txt: describe each line in -v output
count-objects: report garbage files in pack directory too
count-objects: report how much disk space taken by garbage files
Documentation/git-count-objects.txt | 22 +++++++---
builtin/count-objects.c | 41 ++++++++++++++-----
sha1_file.c | 81 +++++++++++++++++++++++++++++++++++--
3 files changed, 127 insertions(+), 17 deletions(-)
--
1.8.1.2.495.g3fdf5d5.dirty
^ permalink raw reply
* [PATCH v2 1/3] git-count-objects.txt: describe each line in -v output
From: Nguyễn Thái Ngọc Duy @ 2013-02-08 3:48 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1360295307-5469-1-git-send-email-pclouds@gmail.com>
The current description requires a bit of guessing (what clause
corresponds to what printed line?) and lacks information, such as
the unit of size and size-pack.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-count-objects.txt | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index 23c80ce..e816823 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -20,11 +20,21 @@ OPTIONS
-------
-v::
--verbose::
- In addition to the number of loose objects and disk
- space consumed, it reports the number of in-pack
- objects, number of packs, disk space consumed by those packs,
- and number of objects that can be removed by running
- `git prune-packed`.
+ Report in more detail:
++
+count: the number of loose objects
++
+size: disk space consumed by loose objects, in KiB
++
+in-pack: the number of in-pack objects
++
+size-pack: disk space consumed by the packs, in KiB
++
+prune-packable: the number of loose objects that are also present in
+the packs. These objects could be pruned using `git prune-packed`.
++
+garbage: the number of files in loose object database that are not
+valid loose objects
GIT
---
--
1.8.1.2.495.g3fdf5d5.dirty
^ permalink raw reply related
* [PATCH v2 2/3] count-objects: report garbage files in pack directory too
From: Nguyễn Thái Ngọc Duy @ 2013-02-08 3:48 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1360295307-5469-1-git-send-email-pclouds@gmail.com>
prepare_packed_git_one() is modified to allow count-objects to hook a
report function to so we don't need to duplicate the pack searching
logic in count-objects.c. When report_pack_garbage is NULL, the
overhead is insignificant.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-count-objects.txt | 4 +-
builtin/count-objects.c | 18 ++++++++-
sha1_file.c | 81 +++++++++++++++++++++++++++++++++++--
3 files changed, 97 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index e816823..1611d7c 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -33,8 +33,8 @@ size-pack: disk space consumed by the packs, in KiB
prune-packable: the number of loose objects that are also present in
the packs. These objects could be pruned using `git prune-packed`.
+
-garbage: the number of files in loose object database that are not
-valid loose objects
+garbage: the number of files in object database that are not valid
+loose objects nor valid packs
GIT
---
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 9afaa88..118b2ae 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -9,6 +9,20 @@
#include "builtin.h"
#include "parse-options.h"
+static unsigned long garbage;
+
+extern void (*report_pack_garbage)(const char *path, int len, const char *name);
+static void real_report_pack_garbage(const char *path, int len, const char *name)
+{
+ if (len && name)
+ error("garbage found: %.*s/%s", len, path, name);
+ else if (!len && name)
+ error("garbage found: %s%s", path, name);
+ else
+ error("garbage found: %s", path);
+ garbage++;
+}
+
static void count_objects(DIR *d, char *path, int len, int verbose,
unsigned long *loose,
off_t *loose_size,
@@ -76,7 +90,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
const char *objdir = get_object_directory();
int len = strlen(objdir);
char *path = xmalloc(len + 50);
- unsigned long loose = 0, packed = 0, packed_loose = 0, garbage = 0;
+ unsigned long loose = 0, packed = 0, packed_loose = 0;
off_t loose_size = 0;
struct option opts[] = {
OPT__VERBOSE(&verbose, N_("be verbose")),
@@ -87,6 +101,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
/* we do not take arguments other than flags for now */
if (argc)
usage_with_options(count_objects_usage, opts);
+ if (verbose)
+ report_pack_garbage = real_report_pack_garbage;
memcpy(path, objdir, len);
if (len && objdir[len-1] != '/')
path[len++] = '/';
diff --git a/sha1_file.c b/sha1_file.c
index 40b2329..cc6ef03 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -21,6 +21,7 @@
#include "sha1-lookup.h"
#include "bulk-checkin.h"
#include "streaming.h"
+#include "dir.h"
#ifndef O_NOATIME
#if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
@@ -1000,6 +1001,54 @@ void install_packed_git(struct packed_git *pack)
packed_git = pack;
}
+/* A hook for count-objects to report invalid files in pack directory */
+void (*report_pack_garbage)(const char *path, int len, const char *name);
+
+static const char *known_pack_extensions[] = { ".pack", ".keep", NULL };
+
+static void report_garbage(struct string_list *list)
+{
+ struct strbuf sb = STRBUF_INIT;
+ struct packed_git *p;
+ int i;
+
+ if (!report_pack_garbage)
+ return;
+
+ sort_string_list(list);
+
+ for (p = packed_git; p; p = p->next) {
+ struct string_list_item *item;
+ if (!p->pack_local)
+ continue;
+ strbuf_reset(&sb);
+ strbuf_add(&sb, p->pack_name,
+ strlen(p->pack_name) - strlen(".pack"));
+ item = string_list_lookup(list, sb.buf);
+ if (!item)
+ continue;
+ /*
+ * string_list_lookup does not guarantee to return the
+ * first matched string if it's duplicated.
+ */
+ while (item - list->items &&
+ !strcmp(item[-1].string, item->string))
+ item--;
+ while (item - list->items < list->nr &&
+ !strcmp(item->string, sb.buf)) {
+ item->util = NULL; /* non-garbage mark */
+ item++;
+ }
+ }
+ for (i = 0; i < list->nr; i++) {
+ struct string_list_item *item = list->items + i;
+ if (!item->util)
+ continue;
+ report_pack_garbage(item->string, 0, item->util);
+ }
+ strbuf_release(&sb);
+}
+
static void prepare_packed_git_one(char *objdir, int local)
{
/* Ensure that this buffer is large enough so that we can
@@ -1009,6 +1058,7 @@ static void prepare_packed_git_one(char *objdir, int local)
int len;
DIR *dir;
struct dirent *de;
+ struct string_list garbage = STRING_LIST_INIT_DUP;
sprintf(path, "%s/pack", objdir);
len = strlen(path);
@@ -1024,14 +1074,37 @@ static void prepare_packed_git_one(char *objdir, int local)
int namelen = strlen(de->d_name);
struct packed_git *p;
- if (!has_extension(de->d_name, ".idx"))
+ if (len + namelen + 1 > sizeof(path)) {
+ if (report_pack_garbage)
+ report_pack_garbage(path, len - 1, de->d_name);
continue;
+ }
+
+ strcpy(path + len, de->d_name);
- if (len + namelen + 1 > sizeof(path))
+ if (!has_extension(de->d_name, ".idx")) {
+ struct string_list_item *item;
+ int i, n;
+ if (!report_pack_garbage)
+ continue;
+ if (is_dot_or_dotdot(de->d_name))
+ continue;
+ for (i = 0; known_pack_extensions[i]; i++)
+ if (has_extension(de->d_name,
+ known_pack_extensions[i]))
+ break;
+ if (!known_pack_extensions[i]) {
+ report_pack_garbage(path, 0, NULL);
+ continue;
+ }
+ n = strlen(path) - strlen(known_pack_extensions[i]);
+ item = string_list_append_nodup(&garbage,
+ xstrndup(path, n));
+ item->util = (void*)known_pack_extensions[i];
continue;
+ }
/* Don't reopen a pack we already have. */
- strcpy(path + len, de->d_name);
for (p = packed_git; p; p = p->next) {
if (!memcmp(path, p->pack_name, len + namelen - 4))
break;
@@ -1047,6 +1120,8 @@ static void prepare_packed_git_one(char *objdir, int local)
install_packed_git(p);
}
closedir(dir);
+ report_garbage(&garbage);
+ string_list_clear(&garbage, 0);
}
static int sort_pack(const void *a_, const void *b_)
--
1.8.1.2.495.g3fdf5d5.dirty
^ permalink raw reply related
* [PATCH v2 3/3] count-objects: report how much disk space taken by garbage files
From: Nguyễn Thái Ngọc Duy @ 2013-02-08 3:48 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1360295307-5469-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
We may do some redundant stat() here, but I don't think it can slow
count-objects down much to worry about.
Documentation/git-count-objects.txt | 2 ++
builtin/count-objects.c | 29 ++++++++++++++++++-----------
2 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index 1611d7c..da6e72e 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -35,6 +35,8 @@ the packs. These objects could be pruned using `git prune-packed`.
+
garbage: the number of files in object database that are not valid
loose objects nor valid packs
++
+size-garbage: disk space consumed by garbage files, in KiB
GIT
---
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 118b2ae..90d476d 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -10,24 +10,33 @@
#include "parse-options.h"
static unsigned long garbage;
+static off_t size_garbage;
extern void (*report_pack_garbage)(const char *path, int len, const char *name);
static void real_report_pack_garbage(const char *path, int len, const char *name)
{
+ struct strbuf sb = STRBUF_INIT;
+ struct stat st;
+
if (len && name)
- error("garbage found: %.*s/%s", len, path, name);
+ strbuf_addf(&sb, "%.*s/%s", len, path, name);
else if (!len && name)
- error("garbage found: %s%s", path, name);
+ strbuf_addf(&sb, "%s%s", path, name);
else
- error("garbage found: %s", path);
+ strbuf_addf(&sb, "%s", path);
+ error(_("garbage found: %s"), sb.buf);
+
+ if (!stat(sb.buf, &st))
+ size_garbage += st.st_size;
+
garbage++;
+ strbuf_release(&sb);
}
static void count_objects(DIR *d, char *path, int len, int verbose,
unsigned long *loose,
off_t *loose_size,
- unsigned long *packed_loose,
- unsigned long *garbage)
+ unsigned long *packed_loose)
{
struct dirent *ent;
while ((ent = readdir(d)) != NULL) {
@@ -59,11 +68,8 @@ static void count_objects(DIR *d, char *path, int len, int verbose,
(*loose_size) += xsize_t(on_disk_bytes(st));
}
if (bad) {
- if (verbose) {
- error("garbage found: %.*s/%s",
- len + 2, path, ent->d_name);
- (*garbage)++;
- }
+ if (verbose)
+ report_pack_garbage(path, len + 2, ent->d_name);
continue;
}
(*loose)++;
@@ -113,7 +119,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
if (!d)
continue;
count_objects(d, path, len, verbose,
- &loose, &loose_size, &packed_loose, &garbage);
+ &loose, &loose_size, &packed_loose);
closedir(d);
}
if (verbose) {
@@ -138,6 +144,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
printf("size-pack: %lu\n", (unsigned long) (size_pack / 1024));
printf("prune-packable: %lu\n", packed_loose);
printf("garbage: %lu\n", garbage);
+ printf("size-garbage: %lu\n", (unsigned long) (size_garbage / 1024));
}
else
printf("%lu objects, %lu kilobytes\n",
--
1.8.1.2.495.g3fdf5d5.dirty
^ permalink raw reply related
* Re: [PATCH] Use __VA_ARGS__ for all of error's arguments
From: Jeff King @ 2013-02-08 4:24 UTC (permalink / raw)
To: Matt Kraai; +Cc: git, Max Horn, Junio C Hamano, Matt Kraai
In-Reply-To: <1360272632-22566-1-git-send-email-kraai@ftbfs.org>
On Thu, Feb 07, 2013 at 01:30:32PM -0800, Matt Kraai wrote:
> From: Matt Kraai <matt.kraai@amo.abbott.com>
>
> QNX 6.3.2 uses GCC 2.95.3 by default, and GCC 2.95.3 doesn't remove the
> comma if the error macro's variable argument is left out.
>
> Instead of testing for a sufficiently recent version of GCC, make
> __VA_ARGS__ match all of the arguments.
Thanks, this looks better than the original (we do not assume a C99
compiler, so just doing this unconditionally would probably break some
other older systems which do not use gcc).
> /*
> * Let callers be aware of the constant return value; this can help
> - * gcc with -Wuninitialized analysis. We have to restrict this trick to
> - * gcc, though, because of the variadic macro and the magic ## comma pasting
> - * behavior. But since we're only trying to help gcc, anyway, it's OK; other
> - * compilers will fall back to using the function as usual.
> + * gcc with -Wuninitialized analysis.
> */
> #if defined(__GNUC__) && ! defined(__clang__)
> -#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
> +#define error(...) (error(__VA_ARGS__), -1)
Should you be dropping most of the comment like this? I would expect it
to be more like:
We have to restrict this trick to gcc, though, because we do not
assume all compilers support variadic macros. But since...
Other than that, I think it is OK. The compiler will still catch
"error()" with no arguments and generate the appropriate diagnostic (in
fact, it is better, because the error is now passing too few args to a
function, not to the macro).
-Peff
^ permalink raw reply
* Re: [PATCH 4/4] git-remote-mediawiki: use Git's Makefile to build the script
From: Jeff King @ 2013-02-08 4:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthieu Moy, git
In-Reply-To: <7vhaln7wkg.fsf@alter.siamese.dyndns.org>
On Thu, Feb 07, 2013 at 11:28:31AM -0800, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
> > The configuration of the install directory is not reused from the
> > toplevel Makefile: we assume Git is already built, hence just call
> > "git --exec-path". This avoids too much surgery in the toplevel Makefile.
> >
> > git-remote-mediawiki.perl can now "use Git;".
> >
> > Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> > ---
>
> Continuing to the comment on 3/4, I wonder if it would be a lot
> simpler and more maintainable if you replaced 1/4 to 3/4 with a
> smaller patch to the top-level Makefile to teach it to munge
> arbitrary path/to/foo.perl to path/to/foo the same way as we do to
> other path/tool.perl that are known to the top-level Makefile
> (similarly, another target to install the resulting path/to/foo at
> an arbitrary place). Then do something like
>
> all::
> $(MAKE) -C ../.. \
> PERL_SCRIPT=contrib/mw-to-git/git-remote-mediawiki.perl \
> build-perl-script
> install::
> $(MAKE) -C ../.. \
> PERL_SCRIPT=contrib/mw-to-git/git-remote-mediawiki.perl \
> install-perl-script
>
> in this step.
That seems much cleaner to me. If done right, it could also let people
put:
CONTRIB_PERL += contrib/mw-to-git/git-remote-mediawiki
or similar into their config.mak, and just get specific contrib bits
built and installed along with the rest of git.
-Peff
^ permalink raw reply
* Re: [PATCH] Use __VA_ARGS__ for all of error's arguments
From: Matt Kraai @ 2013-02-08 4:39 UTC (permalink / raw)
To: Jeff King; +Cc: git, Max Horn, Junio C Hamano, Matt Kraai
In-Reply-To: <20130208042428.GA4157@sigill.intra.peff.net>
On Thu, Feb 07, 2013 at 11:24:28PM -0500, Jeff King wrote:
> Should you be dropping most of the comment like this? I would expect it
> to be more like:
>
> We have to restrict this trick to gcc, though, because we do not
> assume all compilers support variadic macros. But since...
I'll submit a new patch with this change tomorrow.
> Other than that, I think it is OK. The compiler will still catch
> "error()" with no arguments and generate the appropriate diagnostic (in
> fact, it is better, because the error is now passing too few args to a
> function, not to the macro).
Great, thanks for the review.
--
Matt
^ permalink raw reply
* Re: Proposal: branch.<name>.remotepush
From: Jeff King @ 2013-02-08 4:48 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <CALkWK0nA4hQ0VWivk3AVVVq8Rbb-9CpQ9xFsSOsTQtvo4w08rw@mail.gmail.com>
On Thu, Feb 07, 2013 at 09:44:59PM +0530, Ramkumar Ramachandra wrote:
> This has been annoying me for a really long time, but I never really
> got around to scratching this particular itch. I have a very common
> scenario where I fork a project on GitHub. I have two configured
> remotes: origin which points to "git://upstream" and mine which points
> to "ssh://mine". By default, I always want to pull `master` from
> origin and push to mine.
Same here. Even without GitHub, working on git.git I treat Junio as my
"origin", but push to a publishing point.
> Unfortunately, there's only a branch.<name>.remote which specifies
> which remote to use for both pulling and pushing. There's also a
> remote.<name>.pushurl, but I get the feeling that this exists for an
> entirely different reason: when I have a server with a
> highly-available read-only mirror of the repository at
> git://anongit.*, and a less-available committer-only mirror at
> ssh://*.
Yeah, you don't want to use pushurl. It makes the assumption that you
are pushing to the same remote, so when you, e.g., push to the remote's
refs/heads/master, it will update refs/remotes/origin/master. But that's
not right; that ref should be tracking the true origin, not what you
pushed to.
> How about a branch.<name>.remotepush that specifies a special remote
> for pushing, falling back to branch.<name>.remote?
Sure, though I wonder if you really want a per-branch config, or if you
just want remote.pushDefault or similar, so that you do not have to
configure each branch independently as you create it. I'm imagining
lookup rules something like:
1. If we are on branch $b, check branch.$b.pushRemote.
2. If not set, check remote.pushDefault.
3. If not set, check branch.$b.remote.
4. If not set, check remote.default (there was a proposal for this a
few months ago, but it got stalled).
5. If not set, use "origin".
And then fetching could do the same, with s/push/fetch/. In both cases,
if you are not using the new variables, the behavior is the same as
the current behavior.
-Peff
^ permalink raw reply
* Re: [PATCHv2 4/5] Git.pm: add interface for git credential command
From: Jeff King @ 2013-02-08 5:11 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Junio C Hamano, Matthieu Moy, git
In-Reply-To: <78516627e893e54d5aafe0694d1face9a37893de.1360242782.git.mina86@mina86.com>
On Thu, Feb 07, 2013 at 03:01:20PM +0100, Michal Nazarewicz wrote:
> > There are a few disallowed characters, like "\n" in key or value, and
> > "=" in a key. They should never happen unless the caller is buggy, but
> > should we check and catch them here?
>
> I left it as is for now since it's not entairly clear to me what to
> do in all cases. In particular:
>
> - when reading, what to do if the line is " foo = bar ",
According to the spec, whitespace (except for the final newline) is not
significant, and that parses key=" foo ", value=" bar ". The spec could
ignore whitespace on the key side, but I intentionally did not in an
attempt to keep the protocol simple. Your original implementation did
the right thing already.
> - when reading, what to do if the line is "foo=" (ie. empty value),
The empty string is a valid value.
> - when writing, what to do if value is a single space,
Then it's a single space. It's the caller's problem whether that is an
issue or not.
> - when writing, what to do if value ends with a new line,
That's bogus. We cannot represent that value. I'd suggest to simply die,
as it is a bug in the caller (we _could_ try to be nice and assume the
caller accidentally forgot to chomp, but I'd rather be careful than
nice).
> - when writing, what to do if value is empty (currently not printed at all),
I think you should still print it. It's unlikely to matter, but
technically a helper response may override keys (or set them to blank),
and the intermediate state gets sent on to the next helper, if there are
multiple.
> On Thu, Feb 07 2013, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> > I think you should credit git-remote-mediawiki for the code in the
> > commit message. Perhaps have a first "copy/paste" commit, and then an
> > "adaptation" commit to add sort, ^ anchor in regexp, doc and your
> > callback mechanism, but I won't insist on that.
>
> Good point. Creating additional commit is a bit too much for my
> licking, but added note in commit message.
I think that's fine.
> +sub _credential_read {
> + my %credential;
> + my ($reader, $op) = (@_);
> + while (<$reader>) {
> + if (!/^([^=\s]+)=(.*?)\s*$/) {
> + throw Error::Simple("unable to parse git credential $op response:\n$_");
> + }
> + $credential{$1} = $2;
I think this is worse than your previous version. The spec really is as
simple as:
while (<$reader>) {
last if /^$/; # blank line is OK as end-of-credential
/^([^=]+)=(.*)/
or throw Error::Simple(...);
$credential{$1} = {$2};
}
(actually, the spec as written does not explicitly forbid an empty key,
but it is nonsensical, and it might be worth updating the docs).
> +sub _credential_write {
> + my ($credential, $writer) = @_;
> +
> + for my $key (sort {
> + # url overwrites other fields, so it must come first
> + return -1 if $a eq 'url';
> + return 1 if $b eq 'url';
> + return $a cmp $b;
> + } keys %$credential) {
> + if (defined $credential->{$key} && length $credential->{$key}) {
> + print $writer $key, '=', $credential->{$key}, "\n";
> + }
When I mentioned error-checking the format, I really just meant
something like:
$key =~ /[=\n\0]/
and die "BUG: credential key contains invalid characters: $key";
if (defined $credential->{$key}) {
$credential->{$key} =~ /[\n\0]/
and die "BUG: credential value contains invalid characters: $credential->{key}";
print $writer $key, '=', $credential->{$key}, "\n";
}
Those dies should never happen, and are indicative of a bug in the
caller. We can't even represent them in the protocol, so we might as
well alert the user and die rather than trying to guess what the caller
intended.
-Peff
^ permalink raw reply
* Re: [PATCH v4] Add utf8_fprintf helper which returns correct columns
From: Torsten Bögershausen @ 2013-02-08 6:03 UTC (permalink / raw)
To: Jiang Xin
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy, Git List,
Nguyễn Thái Ngọc Duy, Torsten Bögershausen
In-Reply-To: <4ea03e99bad13e2910b137fd3991951244fa23f1.1360289411.git.worldhello.net@gmail.com>
On 08.02.13 03:10, Jiang Xin wrote:
> Since command usages can be translated, they may not align well especially
> when they are translated to CJK. A wrapper utf8_fprintf can help to return
> the correct columns required.
>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> parse-options.c | 5 +++--
> utf8.c | 22 ++++++++++++++++++++++
> utf8.h | 1 +
> 3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 67e98..a6ce9e 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -3,6 +3,7 @@
> #include "cache.h"
> #include "commit.h"
> #include "color.h"
> +#include "utf8.h"
>
> static int parse_options_usage(struct parse_opt_ctx_t *ctx,
> const char * const *usagestr,
> @@ -482,7 +483,7 @@ static int usage_argh(const struct option *opts, FILE *outfile)
> s = literal ? "[%s]" : "[<%s>]";
> else
> s = literal ? " %s" : " <%s>";
> - return fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
> + return utf8_fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
> }
>
> #define USAGE_OPTS_WIDTH 24
> @@ -541,7 +542,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
> if (opts->long_name)
> pos += fprintf(outfile, "--%s", opts->long_name);
> if (opts->type == OPTION_NUMBER)
> - pos += fprintf(outfile, "-NUM");
> + pos += utf8_fprintf(outfile, _("-NUM"));
>
> if ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
> !(opts->flags & PARSE_OPT_NOARG))
> diff --git a/utf8.c b/utf8.c
> index a4ee6..05925 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -430,6 +430,28 @@ int same_encoding(const char *src, const char *dst)
> }
>
> /*
> + * Wrapper for fprintf and returns the total number of columns required
> + * for the printed string, assuming that the string is utf8.
> + */
> +int utf8_fprintf(FILE *stream, const char *format, ...)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + va_list arg;
> + int columns;
> +
> + va_start (arg, format);
> + strbuf_vaddf(&buf, format, arg);
> + va_end (arg);
> +
> + columns = fputs(buf.buf, stream);
> + /* If no error occurs, returns columns really required with utf8_strwidth. */
> + if (0 <= columns)
> + columns = utf8_strwidth(buf.buf);
> + strbuf_release(&buf);
> + return columns;
> +}
> +
I don't think we handle the return code from fputs() correctly.
Please dee below for specifications on fprintf(),
something like the following could do:
int utf8_fprintf(FILE *stream, const char *format, ...)
{
struct strbuf buf = STRBUF_INIT;
va_list arg;
int columns = 0;
va_start (arg, format);
strbuf_vaddf(&buf, format, arg);
va_end (arg);
if (EOF != fputs(buf.buf, stream))
columns = utf8_strwidth(buf.buf);
strbuf_release(&buf);
return columns;
}
And as a side note: would fprintf_strwidth() be a better name?
Linux:
RETURN VALUE
fputc(), putc() and putchar() return the character written as an
unsigned char cast to an int or EOF on error.
puts() and fputs() return a nonnegative number on success, or EOF on
error.
Mac OS:
COMPATIBILITY
fputs() now returns a non-negative number (as opposed to 0) on successful
completion. As a result, many tests (e.g., "fputs() == 0", "fputs() !=
0") do not give the desired result. Use "fputs() != EOF" or "fputs() ==
EOF" to determine success or failure.
Posix:
RETURN VALUE
Upon successful completion, fputs() shall return a non-negative number.
Otherwise, it shall return EOF, set an error indicator for the stream,
and set errno to indicate the error.
^ permalink raw reply
* Re: [RFC] test-lib.sh: No POSIXPERM for cygwin
From: Torsten Bögershausen @ 2013-02-08 6:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, Torsten Bögershausen, git, j6t
In-Reply-To: <7vd2wb7w96.fsf@alter.siamese.dyndns.org>
On 07.02.13 20:35, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>
>> Torsten Bögershausen wrote:
>>> t0070 and t1301 fail when running the test suite under cygwin.
>>> Skip the failing tests by unsetting POSIXPERM.
>> t1301 does not fail for me. (WIN XP (SP3) on NTFS)
> Others run Cygwin with vfat or some other filesystem, and some of
> them do not cope will with POSIXPERM, perhaps?
>
> Not having POSIXPERM by default for Cygwin may be a saner default
> than having one, if we have to pick one.
>
> It may be debatable to have this default as platform attribute,
> though.
>
Yes, 1301 passes on cygwin 1.5, but not on 1.7.
And it breaks on VFAT, for all kind of OS.
Thanks for comments, I'll put more investigations on my todo stack.
/Torsten
^ permalink raw reply
* Re: Proposal: branch.<name>.remotepush
From: Junio C Hamano @ 2013-02-08 6:08 UTC (permalink / raw)
To: Jeff King; +Cc: Ramkumar Ramachandra, Git List
In-Reply-To: <20130208044836.GC4157@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Feb 07, 2013 at 09:44:59PM +0530, Ramkumar Ramachandra wrote:
>
>> This has been annoying me for a really long time, but I never really
>> got around to scratching this particular itch. I have a very common
>> scenario where I fork a project on GitHub. I have two configured
>> remotes: origin which points to "git://upstream" and mine which points
>> to "ssh://mine". By default, I always want to pull `master` from
>> origin and push to mine.
>
> Same here. Even without GitHub, working on git.git I treat Junio as my
> "origin", but push to a publishing point.
The "you fetch from and push to the same place" semantics that
associates a branch to a single remote was primarily done for people
coming from CVS/SVN background [*1*]. I think the triangle
arrangement where you want to have "this is where I fetch from and
integrate with, and that is where I publish" is more common among
the Git users these days.
How best to express the triangle is somewhat tricky, but I think it
is sensible to say you have "origin" that points to your upstream
(i.e. me), and "peff" that points to your publishing point, in other
words, make it explicit that the user deals with two remotes. Then
have push.default name the remote "peff", so that "git push" goes to
that remote by default (and have "git fetch/pull" go to "origin).
You will have two sets of remote tracking branches (one from "origin"
that your push will never pretend to have fetched immediately after
finishing, the other from "peff" that keeps track of what you pushed
the last time).
Of course, some people may have "I use this and that branches to
interact with upstream X while I use these other branches to
interacct with upstream Y, and all of them push to different
places", and supporting that may need complex per branch "On this
branch fetch from and integrate with remote X, and push to remote Z"
settings, but as you said, "I fetch from and integrate with X, and
result is pushed out to Y" should be the most common, and it would
be desirable to have a simple way to express it with just a single
new configuration variable.
[Footnote]
*1* It also happens to work reasonably well for people like Linus
and I with the "I pull from random places, I locally integrate and I
publish the results" workflow, because we are trained to think that
it is not just being lazy but simply meaningless to say "git pull"
without saying "fetch and integrate _what_ and from _whom_", and
that is only because we do not have a fixed upstream. Linus and I
would practically never fetch from "origin", i.e. from ourselves.
^ permalink raw reply
* Re: [PATCH v4] Add utf8_fprintf helper which returns correct columns
From: Torsten Bögershausen @ 2013-02-08 6:13 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Jiang Xin, Junio C Hamano, Nguyễn Thái Ngọc Duy,
Git List, Nguyễn Thái Ngọc Duy
In-Reply-To: <51149542.8060307@web.de>
(Sorry for confusing: I should have written:)
Please see below for specifications on fputs()
Linux:
RETURN VALUE
fputc(), putc() and putchar() return the character written as an unsigned char cast to an int or EOF on error.
puts() and fputs() return a nonnegative number on success, or EOF on error.
Mac OS:
COMPATIBILITY
fputs() now returns a non-negative number (as opposed to 0) on successful completion. As a result, many tests (e.g., "fputs() == 0", "fputs() != 0") do not give the desired result.
Use "fputs() != EOF" or "fputs() == EOF" to determine success or failure.
Posix:
RETURN VALUE
Upon successful completion, fputs() shall return a non-negative number. Otherwise, it shall return EOF, set an error indicator for the stream, and set errno to indicate the error.
^ permalink raw reply
* Re: [PATCHv6] Add contrib/credentials/netrc with GPG support
From: Junio C Hamano @ 2013-02-08 6:15 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: Jeff King, git
In-Reply-To: <87mwvfbmgi.fsf@lifelogs.com>
Ted Zlatanov <tzz@lifelogs.com> writes:
> I agree this Makefile is not a good test to ship out. It was my quickie
> test rig that I should have reworked before adding to the patch. Sorry.
Nothing to be sorry about. Starting with quick-and-dirty and
polishing for public consumption is what the review cycle is about,
and we are here to help that process.
> I see contrib/subtree/t and contrib/mw-to-git/t that I could copy. The
> test will have a few files to parse, and will be able to compare the
> expected to the actual output. Does that sound like a good plan?
Yup.
Thanks.
^ permalink raw reply
* Re: [PATCHv6] Add contrib/credentials/netrc with GPG support
From: Jeff King @ 2013-02-08 6:18 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: Junio C Hamano, git
In-Reply-To: <876226p97h.fsf_-_@lifelogs.com>
On Tue, Feb 05, 2013 at 07:38:58PM -0500, Ted Zlatanov wrote:
> Add Git credential helper that can parse netrc/authinfo files.
>
> This credential helper supports multiple files, returning the first one
> that matches. It checks file permissions and owner. For *.gpg files,
> it will run GPG to decrypt the file.
>
> Signed-off-by: Ted Zlatanov <tzz@lifelogs.com>
> + # the following check is copied from Net::Netrc, for non-GPG files
> + # OS/2 and Win32 do not handle stat in a way compatable with this check :-(
s/compatable/compatible/
You mention os/2 and Win32 here, but the check has more:
> + unless ($gpgmode || $options{insecure} ||
> + $^O eq 'os2'
> + || $^O eq 'MSWin32'
> + || $^O eq 'MacOS'
> + || $^O =~ /^cygwin/) {
Does MacOS really not handle stat? Or is this old MacOS, not OS X?
> +sub load_netrc {
> [...]
> + foreach my $nentry (@netrc_entries) {
> + my %entry;
> + my $num_port;
> +
> + if (!defined $nentry->{machine}) {
> + next;
> + }
> + if (defined $nentry->{port} && $nentry->{port} =~ m/^\d+$/) {
> + $num_port = $nentry->{port};
> + delete $nentry->{port};
> + }
> +
> + # create the new entry for the credential helper protocol
> + $entry{$options{tmap}->{$_}} = $nentry->{$_} foreach keys %$nentry;
> +
> + # for "host X port Y" where Y is an integer (captured by
> + # $num_port above), set the host to "X:Y"
> + if (defined $entry{host} && defined $num_port) {
> + $entry{host} = join(':', $entry{host}, $num_port);
> + }
So this will convert:
machine foo port smtp
in the netrc into (protocol => "smtp", host => "foo"), but:
machine foo port 25
into (protocol => undef, host => "foo:25"), right? That makes sense to
me.
> +sub net_netrc_loader {
> [...]
I won't comment here, as I know very little about netrc (I always
thought it was line-oriented, too!) and Junio has covered it.
> +# takes the search tokens and then a list of entries
> +# each entry is a hash reference
> +sub find_netrc_entry {
> + my $query = shift @_;
> +
> + ENTRY:
> + foreach my $entry (@_)
> + {
> + my $entry_text = join ', ', map { "$_=$entry->{$_}" } keys %$entry;
> + foreach my $check (sort keys %$query) {
> + if (defined $query->{$check}) {
> + log_debug("compare %s [%s] to [%s] (entry: %s)",
> + $check,
> + $entry->{$check},
> + $query->{$check},
> + $entry_text);
> + unless ($query->{$check} eq $entry->{$check}) {
> + next ENTRY;
> + }
> + } else {
> + log_debug("OK: any value satisfies check $check");
> + }
This looks right to me.
> +sub print_credential_data {
I don't know if you want to take the hit of relying on Git.pm (it is
nice for the helper to be totally standalone and copy-able), but one
obvious possible refactor would be to use the credential read/write
functions recently added there. I'm OK with not doing that, though.
> + my $entry = shift @_;
> + my $query = shift @_;
> +
> + log_debug("entry has passed all the search checks");
> + TOKEN:
> + foreach my $git_token (sort keys %$entry) {
> + log_debug("looking for useful token $git_token");
> + # don't print unknown (to the credential helper protocol) tokens
> + next TOKEN unless exists $query->{$git_token};
> +
> + # don't print things asked in the query (the entry matches them)
> + next TOKEN if defined $query->{$git_token};
> +
> + log_debug("FOUND: $git_token=$entry->{$git_token}");
> + printf "%s=%s\n", $git_token, $entry->{$git_token};
> + }
Printf? Bleh, isn't this supposed to be perl? :P
I don't see anything wrong from the credential-handling side of things.
As I said, I didn't look closely at the netrc parsing bits. From my
reading of "perldoc macos", the answer to my question above is "yes,
stat doesn't work on MacOS Classic". So I think the script itself is
fine.
In your tests:
> +++ b/contrib/credential/netrc/Makefile
> @@ -0,0 +1,12 @@
> +test_netrc:
> + @(echo "bad data" | ./git-credential-netrc -f A -d -v) || echo "Bad invocation test, ignoring
> failure"
> + @echo "=> Silent invocation... nothing should show up here with a missing file"
> + @echo "bad data" | ./git-credential-netrc -f A get
> + @echo "=> Back to noisy: -v and -d used below, missing file"
> + echo "bad data" | ./git-credential-netrc -f A -d -v get
> + @echo "=> Look for any entry in the default file set"
> + echo "" | ./git-credential-netrc -d -v get
> + @echo "=> Look for github.com in the default file set"
> + echo "host=google.com" | ./git-credential-netrc -d -v get
> + @echo "=> Look for a nonexistent machine in the default file set"
> + echo "host=korovamilkbar" | ./git-credential-netrc -d -v get
You are depending on whatever the user has in their ~/.netrc, no?
Wouldn't it make more sense to ship a sample netrc and run all of the
tests with "-f netrc.example"?
It may also be worth building on top of the regular git test harness.
It's more work, but the resulting code (and the output) will be much
more readable.
-Peff
^ permalink raw reply
* Re: How to diff 2 file revisions with gitk
From: Sitaram Chamarty @ 2013-02-08 6:21 UTC (permalink / raw)
To: R. Diez; +Cc: git@vger.kernel.org
In-Reply-To: <1360166273.33888.YahooMailNeo@web171204.mail.ir2.yahoo.com>
On Wed, Feb 6, 2013 at 9:27 PM, R. Diez <rdiezmail-buspirate@yahoo.de> wrote:
> Hi there:
>
> I asked a few days ago whether I could easily diff 2 file revisions with the mouse in gitk, but I got no reply yet, see here:
>
>
> How to diff two file revisions with the mouse (with gitk)
> https://groups.google.com/forum/#!topic/git-users/9znsQsTB0dE
>
> I am hoping that it was the wrong mailing list, and this one the right one. 8-)
>
> Here is the full question text again:
>
> --------8<--------8<--------8<--------8<--------
>
> I would like to start gitk, select with the mouse 2
> revisions of some file and then compare them, hopefully with an external
> diff tool, very much like I am used to with WinCVS.
>
> The closest I
> got is to start gitk with a filename as an argument, in order to
> restrict the log to that one file. Then I right-click on a commit (a
> file revision) and choose "Mark this commit". However, if I right-click
> on another commit and choose "Compare with marked commit", I get a full
> commit diff with all files, and not just the file I specified on the
> command-line arguments.
>
> Selecting a filename in the "Tree" view and choosing "Highlight this only", as I found on the Internet, does not seem to help.
>
> I have git 1.7.9 (on Cygwin). Can someone help?
>
> By the way, it would be nice if gitk could launch the external diff tool from the "Compare with marked commit" option too.
I don't know if I misunderstood the whole question because the answer
is very simple.
- start gitk
- left click the newer commit
- scroll to the older commit
- right click the older commit and choose "Diff this -> selected"
- in the bottom right pane, pick any file, right click, and choose
"External diff".
^ permalink raw reply
* Re: Proposal: branch.<name>.remotepush
From: Jeff King @ 2013-02-08 6:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramkumar Ramachandra, Git List
In-Reply-To: <7vliaz49sf.fsf@alter.siamese.dyndns.org>
On Thu, Feb 07, 2013 at 10:08:48PM -0800, Junio C Hamano wrote:
> How best to express the triangle is somewhat tricky, but I think it
> is sensible to say you have "origin" that points to your upstream
> (i.e. me), and "peff" that points to your publishing point, in other
> words, make it explicit that the user deals with two remotes. Then
> have push.default name the remote "peff", so that "git push" goes to
> that remote by default (and have "git fetch/pull" go to "origin).
> You will have two sets of remote tracking branches (one from "origin"
> that your push will never pretend to have fetched immediately after
> finishing, the other from "peff" that keeps track of what you pushed
> the last time).
Exactly. That is what I have set up now, except that I have to type "git
push peff" because there is no such push.default (with the minor nit
that push.default does something else, so the config should be called
remote.pushDefault or something). The entirety of the feature would be
saving the user from the annoyance of:
$ git push
fatal: remote error:
You can't push to git://github.com/gitster/git.git
Use git@github.com:gitster/git.git
[doh! Stupid git, why don't you do what I mean, not what I say?]
$ git push peff
... it works ...
> Of course, some people may have "I use this and that branches to
> interact with upstream X while I use these other branches to
> interacct with upstream Y, and all of them push to different
> places", and supporting that may need complex per branch "On this
> branch fetch from and integrate with remote X, and push to remote Z"
> settings, but as you said, "I fetch from and integrate with X, and
> result is pushed out to Y" should be the most common, and it would
> be desirable to have a simple way to express it with just a single
> new configuration variable.
Right. Frankly, I do not care that much about the per-branch push remote
myself. In the rules I gave earlier, that was my complete
backwards-compatible vision, so that we do not paint ourselves into a
corner compatibility-wise when somebody wants it later. Just
implementing the default push remote part would be a fine first step.
I also indicated in my rules that we could have a branch.*.fetchRemote,
as well, but I do not think it is strictly necessary. I think the
non-specific branch.*.remote could continue to be used for fetching, and
as a backup when the push-specific variables are not set.
> *1* It also happens to work reasonably well for people like Linus
> and I with the "I pull from random places, I locally integrate and I
> publish the results" workflow, because we are trained to think that
> it is not just being lazy but simply meaningless to say "git pull"
> without saying "fetch and integrate _what_ and from _whom_", and
> that is only because we do not have a fixed upstream. Linus and I
> would practically never fetch from "origin", i.e. from ourselves.
Right, I think "git pull" is more useful in a centralized repo setting
where there is one branch and one repo, so there is no "what and whom"
to specify. Personally I do not use it much at all, as I do a separate
fetch, inspect, and merge, but that is somewhat orthogonal to your
reasons. :)
-Peff
^ permalink raw reply
* Re: Proposal: branch.<name>.remotepush
From: Junio C Hamano @ 2013-02-08 6:45 UTC (permalink / raw)
To: Jeff King; +Cc: Ramkumar Ramachandra, Git List
In-Reply-To: <7vliaz49sf.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> .... I think the triangle
> arrangement where you want to have "this is where I fetch from and
> integrate with, and that is where I publish" is more common among
> the Git users these days.
Another thing to know about is that the recent move to change the
behaviour of "git push" to work only on one branch per default may
have to be polished and strengthened a bit.
Originally, the encouraged workflow was to perfect _everything_ that
you would push out and then with a single "git push" to publish
everything at the same time. Both the "matching" behaviour of "git
push" which was the default, and the set of push refspecs that is to
be defined per remote, were ways to discourage "Work on one branch,
think it is OK, hastily push only that branch out, switch to another
branch, rinse, repeat".
To support a triangular arrangement well, there may need some
thinking on what $branch@{upstream} means. The original intent of
the upstream mode specified for "push.default" is push the result
back to what you based your work on, but in a triangular arrangement
that is no longer true. You may be keeping up with my 'master' by
constantly rebasing and then pushing out the result to your 'frotz'
topic. You want to have a lazy "git fetch" to fetch from my
'master' (i.e. upstream), and have remotes/origin/master to keep
track of it. You want to see "git rebase" to pay attention to the
updates to remotes/origin/master when figuring out where you forked.
But at the same time, you want a lazy "git push" to go to your
push.defaultTo repository (i.e. your publish point) and update your
'frotz' branch there---remotes/origin/master should not come into
the picture at all. But the upstream and simple modes want to pay
attention to branch.$name.merge, which is all about the "fetch and
integrate" side of the equation.
^ permalink raw reply
* [RFC/PATCH] Introduce branch.<name>.pushremote
From: Ramkumar Ramachandra @ 2013-02-08 7:19 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jonathan Nieder, Jeff King
This new configuration variable overrides the remote in
`branch.<name>.remote` for pushes. It is useful in the typical
scenario, where the remote I'm pulling from is not the remote I'm
pushing to. Although `remote.<name>.pushurl` is similar, it does not
serve the purpose as the URL would lack corresponding remote tracking
branches.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
This is a first cut. There's code duplication at the moment, but I'm
currently trying to figure out which other remote_get() calls to
replace with pushremote_get(). Comments are welcome.
I will leave it to future patches to do the following things:
1. Fix the status output to be more meaningful when pushremote is
set. At the moment, I'm thinking statuses like [pull: 4 behind,
push: 3 ahead] will make sense.
2. Introduce a remote.pushDefault (peff)
3. Introduce a remote.default (peff)
Documentation/config.txt | 6 ++++++
builtin/push.c | 2 +-
remote.c | 41 +++++++++++++++++++++++++++++++++++++++++
remote.h | 2 ++
4 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9b11597..0b3b1f8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -727,6 +727,12 @@ branch.<name>.remote::
remote to fetch from/push to. It defaults to `origin` if no remote is
configured. `origin` is also used if you are not on any branch.
+branch.<name>.pushremote::
+ When in branch <name>, it tells 'git push' which remote to
+ push to. It falls back to `branch.<name>.remote`, and
+ defaults to `origin` if no remote is configured. `origin` is
+ also used if you are not on any branch.
+
branch.<name>.merge::
Defines, together with branch.<name>.remote, the upstream branch
for the given branch. It tells 'git fetch'/'git pull'/'git rebase' which
diff --git a/builtin/push.c b/builtin/push.c
index 42b129d..d447a80 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -322,7 +322,7 @@ static int push_with_options(struct transport *transport, int flags)
static int do_push(const char *repo, int flags)
{
int i, errs;
- struct remote *remote = remote_get(repo);
+ struct remote *remote = pushremote_get(repo);
const char **url;
int url_nr;
diff --git a/remote.c b/remote.c
index e53a6eb..d6fcfc0 100644
--- a/remote.c
+++ b/remote.c
@@ -48,6 +48,7 @@ static int branches_nr;
static struct branch *current_branch;
static const char *default_remote_name;
+static const char *pushremote_name;
static int explicit_default_remote_name;
static struct rewrites rewrites;
@@ -363,6 +364,12 @@ static int handle_config(const char *key, const char *value, void *cb)
default_remote_name = branch->remote_name;
explicit_default_remote_name = 1;
}
+ } else if (!strcmp(subkey, ".pushremote")) {
+ if (!value)
+ return config_error_nonbool(key);
+ branch->pushremote_name = xstrdup(value);
+ if (branch == current_branch)
+ pushremote_name = branch->pushremote_name;
} else if (!strcmp(subkey, ".merge")) {
if (!value)
return config_error_nonbool(key);
@@ -700,6 +707,40 @@ struct remote *remote_get(const char *name)
return ret;
}
+struct remote *pushremote_get(const char *name)
+{
+ struct remote *ret;
+ int name_given = 0;
+
+ read_config();
+ if (name)
+ name_given = 1;
+ else {
+ if (pushremote_name) {
+ name = pushremote_name;
+ name_given = 1;
+ } else {
+ name = default_remote_name;
+ name_given = explicit_default_remote_name;
+ }
+ }
+
+ ret = make_remote(name, 0);
+ if (valid_remote_nick(name)) {
+ if (!valid_remote(ret))
+ read_remotes_file(ret);
+ if (!valid_remote(ret))
+ read_branches_file(ret);
+ }
+ if (name_given && !valid_remote(ret))
+ add_url_alias(ret, name);
+ if (!valid_remote(ret))
+ return NULL;
+ ret->fetch = parse_fetch_refspec(ret->fetch_refspec_nr, ret->fetch_refspec);
+ ret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);
+ return ret;
+}
+
int remote_is_configured(const char *name)
{
int i;
diff --git a/remote.h b/remote.h
index 251d8fd..aa42ff5 100644
--- a/remote.h
+++ b/remote.h
@@ -51,6 +51,7 @@ struct remote {
};
struct remote *remote_get(const char *name);
+struct remote *pushremote_get(const char *name);
int remote_is_configured(const char *name);
typedef int each_remote_fn(struct remote *remote, void *priv);
@@ -130,6 +131,7 @@ struct branch {
const char *refname;
const char *remote_name;
+ const char *pushremote_name;
struct remote *remote;
const char **merge_name;
--
1.8.1.2.545.g2f19ada.dirty
^ permalink raw reply related
* Re: [PATCH v4] Add utf8_fprintf helper which returns correct columns
From: Jiang Xin @ 2013-02-08 7:20 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy, Git List,
Nguyễn Thái Ngọc Duy
In-Reply-To: <51149542.8060307@web.de>
2013/2/8 Torsten Bögershausen <tboegi@web.de>:
> On 08.02.13 03:10, Jiang Xin wrote:
>> + /* If no error occurs, returns columns really required with utf8_strwidth. */
>> + if (0 <= columns)
>> + columns = utf8_strwidth(buf.buf);
>> + strbuf_release(&buf);
>> + return columns;
>> +}
>> +
>
> I don't think we handle the return code from fputs() correctly.
>
> Please dee below for specifications on fprintf(),
> something like the following could do:
>
> int utf8_fprintf(FILE *stream, const char *format, ...)
> {
> struct strbuf buf = STRBUF_INIT;
> va_list arg;
> int columns = 0;
>
> va_start (arg, format);
> strbuf_vaddf(&buf, format, arg);
> va_end (arg);
>
> if (EOF != fputs(buf.buf, stream))
> columns = utf8_strwidth(buf.buf);
> strbuf_release(&buf);
> return columns;
> }
As fputs() returns a non-negative number (as opposed to 0) on
successful completion,
Test fputs() return value as "fputs() >=0" is correct, while "fputs()
== 0", "fputs() != 0"
are wrong. I think it's OK, must I send a new re-roll for this?
EOF is defined as (-1) in stdio.h:
#define EOF (-1)
> And as a side note: would fprintf_strwidth() be a better name?
This is a nice candidate.
--
Jiang Xin
^ permalink raw reply
* Re: Proposal: branch.<name>.remotepush
From: Jonathan Nieder @ 2013-02-08 7:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramkumar Ramachandra, Michael Schubert, Git List
In-Reply-To: <7v38x766b2.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> I'd actually see this as Gerrit being weird.
>
> If it wants to quarantine a commit destined to the "master" branch,
> couldn't it just let people push to "master" and then internally
> update "for/master" instead?
It is because pushing doesn't update refs/heads/master. Instead, it
starts a code review.
Suppose Gerrit allows starting a new code review by pushing to
refs/heads/master. It sounds okay if I squint --- it's just a very
slow asynchronous ref update, right? Let's see:
$ git clone <gerrit server> test
Cloning into 'test'...
$ echo hi >greeting
$ git add greeting
$ git commit -q -m 'hello'
$ git push origin master
[...]
remote: New Changes:
remote: <gerrit server>/r/1234
remote:
To <url>
ea4cb77b..9117390e master -> master
$ : walk away, forget what I was doing
$ git fetch origin
From <url>
+ 9117390...ea4cb77 master -> origin/master (forced update)
"Wait, why did the remote rewind?"
Regards,
Jonathan
^ permalink raw reply
* Re: Proposal: branch.<name>.remotepush
From: Junio C Hamano @ 2013-02-08 8:16 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Ramkumar Ramachandra, Michael Schubert, Git List
In-Reply-To: <20130208074813.GA7337@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> "Wait, why did the remote rewind?"
Oh, I am very well aware of that glitch.
"git push" has this hack to pretend as if the pusher immediately
turned around and fetched from the remote.
It shouldn't have been made to do so unconditionally; instead it
should have been designed to give the pushee a way to optionally
tell you "I acccept this push, but you may not see it to be updated
to that exact value you pushed when you fetched from me right now".
The hack is not my design; it was not even something I accepted
without complaints, so I can badmouth about it all I want without
hesitation ;-)
More importantly, we could fix it if we wanted to.
^ permalink raw reply
* Re: [RFC/PATCH] Introduce branch.<name>.pushremote
From: Junio C Hamano @ 2013-02-08 8:21 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Jonathan Nieder, Jeff King
In-Reply-To: <1360307982-20027-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Comments are welcome.
As the first cut, I would have expected the series to start from
more general (not "only this branch"), with later follow-up to let
more specific configuration.
Also I'd prefer to see the "push" semantics (e.g. "what does
upstream mean in this new world order?") designed better first.
^ permalink raw reply
* [RFC/PATCH] Introduce remote.pushdefault
From: Ramkumar Ramachandra @ 2013-02-08 9:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder, Jeff King
In-Reply-To: <7v1ucr43mk.fsf@alter.siamese.dyndns.org>
This new configuration variable overrides branch-specific
configuration `branch.<name>.remote` for pushes. It is useful in the
typical scenario, where the remote I'm pulling from is not the remote
I'm pushing to.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
Junio C Hamano wrote:
> As the first cut, I would have expected the series to start from
> more general (not "only this branch"), with later follow-up to let
> more specific configuration.
Doesn't that follow trivially from my previous patch? I'm looking
for comments on how best to share code between pushremote_get()/
remote_get(), and on other remote_get() callsites.
> Also I'd prefer to see the "push" semantics (e.g. "what does
> upstream mean in this new world order?") designed better first.
Why should the meaning of upstream change? We'd probably like to
introduce something like a branch@{downstream} pointing to the push
remote ref sometime in the future though. Wait, should it always be
called downstream?
Documentation/config.txt | 4 ++++
builtin/push.c | 2 +-
remote.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
remote.h | 1 +
4 files changed, 49 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9b11597..82a4a78 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1884,6 +1884,10 @@ receive.updateserverinfo::
If set to true, git-receive-pack will run git-update-server-info
after receiving data from git-push and updating refs.
+remote.pushdefault::
+ The remote to push to by default. Overrides the
+ branch-specific configuration `branch.<name>.remote`.
+
remote.<name>.url::
The URL of a remote repository. See linkgit:git-fetch[1] or
linkgit:git-push[1].
diff --git a/builtin/push.c b/builtin/push.c
index 42b129d..d447a80 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -322,7 +322,7 @@ static int push_with_options(struct transport *transport, int flags)
static int do_push(const char *repo, int flags)
{
int i, errs;
- struct remote *remote = remote_get(repo);
+ struct remote *remote = pushremote_get(repo);
const char **url;
int url_nr;
diff --git a/remote.c b/remote.c
index e53a6eb..08bb803 100644
--- a/remote.c
+++ b/remote.c
@@ -48,6 +48,7 @@ static int branches_nr;
static struct branch *current_branch;
static const char *default_remote_name;
+static const char *pushremote_name;
static int explicit_default_remote_name;
static struct rewrites rewrites;
@@ -349,6 +350,14 @@ static int handle_config(const char *key, const char *value, void *cb)
const char *subkey;
struct remote *remote;
struct branch *branch;
+ if (!prefixcmp(key, "remote.")) {
+ name = key + 7;
+ if (!strcmp(name, "pushdefault")) {
+ if (!value)
+ return config_error_nonbool(key);
+ pushremote_name = xstrdup(value);
+ }
+ }
if (!prefixcmp(key, "branch.")) {
name = key + 7;
subkey = strrchr(name, '.');
@@ -388,8 +397,6 @@ static int handle_config(const char *key, const char *value, void *cb)
add_instead_of(rewrite, xstrdup(value));
}
}
- if (prefixcmp(key, "remote."))
- return 0;
name = key + 7;
if (*name == '/') {
warning("Config remote shorthand cannot begin with '/': %s",
@@ -700,6 +707,40 @@ struct remote *remote_get(const char *name)
return ret;
}
+struct remote *pushremote_get(const char *name)
+{
+ struct remote *ret;
+ int name_given = 0;
+
+ read_config();
+ if (name)
+ name_given = 1;
+ else {
+ if (pushremote_name) {
+ name = pushremote_name;
+ name_given = 1;
+ } else {
+ name = default_remote_name;
+ name_given = explicit_default_remote_name;
+ }
+ }
+
+ ret = make_remote(name, 0);
+ if (valid_remote_nick(name)) {
+ if (!valid_remote(ret))
+ read_remotes_file(ret);
+ if (!valid_remote(ret))
+ read_branches_file(ret);
+ }
+ if (name_given && !valid_remote(ret))
+ add_url_alias(ret, name);
+ if (!valid_remote(ret))
+ return NULL;
+ ret->fetch = parse_fetch_refspec(ret->fetch_refspec_nr, ret->fetch_refspec);
+ ret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);
+ return ret;
+}
+
int remote_is_configured(const char *name)
{
int i;
diff --git a/remote.h b/remote.h
index 251d8fd..99a437f 100644
--- a/remote.h
+++ b/remote.h
@@ -51,6 +51,7 @@ struct remote {
};
struct remote *remote_get(const char *name);
+struct remote *pushremote_get(const char *name);
int remote_is_configured(const char *name);
typedef int each_remote_fn(struct remote *remote, void *priv);
--
1.8.1.3.535.ga923c31.dirty
^ permalink raw reply related
* Re: Proposal: branch.<name>.remotepush
From: Jeff King @ 2013-02-08 9:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramkumar Ramachandra, Git List
In-Reply-To: <7vd2wb483w.fsf@alter.siamese.dyndns.org>
On Thu, Feb 07, 2013 at 10:45:07PM -0800, Junio C Hamano wrote:
> To support a triangular arrangement well, there may need some
> thinking on what $branch@{upstream} means. The original intent of
> the upstream mode specified for "push.default" is push the result
> back to what you based your work on, but in a triangular arrangement
> that is no longer true.
I don't think that "upstream" or "simple" push settings really make
sense in such a triangular arrangement. And IMHO, that's OK. They
reflect a much simpler view of the world than git is capable of
supporting. So "simple" works OK as a default, and people can move to
"matching" (or "current", or even a custom refspec) once they have are
ready to take advantage of a more advanced topology/workflow.
We have the problem now that new users do not necessarily understand the
matching strategy, or why it is useful, and get confused. When we move
to "simple", we may be switching to a world where the early part of the
learning curve is more gentle for those users, but they eventually run
across the steeper part when they want to adjust their workflow (i.e.,
they will eventually learn about non-symmetric repo topologies because
those are part of many useful workflows).
But I think it's a good thing to push that part of the learning curve
out, because:
1. Some people may stay in the centralized view their whole lives and
never care.
2. It will make more sense to them, because they'll understand how it
fits into what they're trying to do, rather than viewing it as an
arcane and senseless default.
There may be some confusion as people hit that learning point. I won't
be surprised if we end up adding more advice.* messages in certain cases
to guide people to adjusting their push.default. But I'm just as happy
to wait until people start hitting the confusion point in practice, and
we can see more clearly when that advice should trigger, and what it
should say.
Unless you have ideas now, of course, in which case I'm happy to hear
them. :)
-Peff
^ 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