* Re: [PATCH 3/5] check-ref-format: Abolish leak of collapsed refname
From: Michael Haggerty @ 2016-12-19 11:09 UTC (permalink / raw)
To: Ian Jackson, git
In-Reply-To: <20161104191358.28812-4-ijackson@chiark.greenend.org.uk>
On 11/04/2016 08:13 PM, Ian Jackson wrote:
> collapse_slashes always returns a value from xmallocz.
>
> Right now this leak is not very interesting, since we only call
> check_one_ref_format once.
>
> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
> ---
> builtin/check-ref-format.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index f12c19c..020ebe8 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -63,8 +63,10 @@ static int check_one_ref_format(const char *refname)
> : check_refname_format(refname, flags);
> if (got)
> return 1;
If the function returns via the line above, then the memory will still
be leaked.
> - if (normalize)
> + if (normalize) {
> printf("%s\n", refname);
> + free((void*)refname);
> + }
> }
>
> int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
>
Michael
^ permalink raw reply
* Re: [PATCH v2 02/21] config: add git_config_get_split_index()
From: Duy Nguyen @ 2016-12-19 11:14 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161217145547.11748-3-chriscool@tuxfamily.org>
On Sat, Dec 17, 2016 at 03:55:28PM +0100, Christian Couder wrote:
> diff --git a/config.c b/config.c
> index 2eaf8ad77a..c1343bbb3e 100644
> --- a/config.c
> +++ b/config.c
> @@ -1709,6 +1709,16 @@ int git_config_get_untracked_cache(void)
> return -1; /* default value */
> }
>
> +int git_config_get_split_index(void)
> +{
> + int val = -1;
Is it redundant to set default value here because it's not used
anywhere? The "return val;" will always have the new value from
git_config_. And you don't use "val" in error case.
> +
> + if (!git_config_get_maybe_bool("core.splitindex", &val))
> + return val;
> +
> + return -1; /* default value */
> +}
> +
> NORETURN
> void git_die_config_linenr(const char *key, const char *filename, int linenr)
> {
> --
> 2.11.0.49.g2414764.dirty
>
^ permalink raw reply
* Re: [PATCH v2 04/21] read-cache: add and then use tweak_split_index()
From: Duy Nguyen @ 2016-12-19 11:18 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161217145547.11748-5-chriscool@tuxfamily.org>
On Sat, Dec 17, 2016 at 03:55:30PM +0100, Christian Couder wrote:
> +static void tweak_split_index(struct index_state *istate)
> +{
> + switch (git_config_get_split_index()) {
> + case -1: /* unset: do nothing */
> + break;
> + case 0: /* false */
> + remove_split_index(istate);
> + break;
> + case 1: /* true */
> + add_split_index(istate);
> + break;
> + default: /* unknown value: do nothing */
Probably should die("BUG:") here since it looks to me like
git_config_maybe_bool() (inside git_config_get_split_index) in this
case has been updated to return more values than we can handle. And we
need to know about that so we can handle it properly.
> + break;
> + }
> +}
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
From: Johannes Schindelin @ 2016-12-19 10:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqinqn6c41.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 13 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> While I think it would make it easier for people to experiment and
> >> build on if the topic is merged to 'next', I am at the same time a
> >> bit reluctant to merge an unproven new topic that introduces a new
> >> file format, which we may end up having to support til the end of
> >> time. It is likely that to support a "prime clone from CDN", it
> >> would need a lot more than just "these are the heads and the pack
> >> data is over there", so this may not be sufficient.
> >>
> >> Will discard.
> >
> > You could mark it as experimental, subject to change, and merge it to
> > `next` safely.
>
> Are you planning, or do you know somebody who plans to use that code
> soonish?
I am too swamped with other things (most importantly, automate the
identification of the as-of-recent-quite-frequent breakages reported by my
build jobs).
I know that one of my colleagues wanted to have a look at it, and so I
thought that having it as an experimental feature that I could even
integrate into Git for Windows for a wider audience could help justify
alotting the time.
> Otherwise I'd prefer to drop it---at this point, the series is merely
> "just because we can", not "because we need it to further improve this
> or that".
Oh, I thought that this was meant as a starting point for anybody
interested in playing with resumable clones or with easing server loads.
In any case, I just wanted to be sure that you considered making it an
experimental feature instead of dropping it. Just in case that you did not
think of that as a possibility.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 5/5] check-ref-format: New --stdin option
From: Michael Haggerty @ 2016-12-19 11:22 UTC (permalink / raw)
To: Ian Jackson, git
In-Reply-To: <20161104191358.28812-6-ijackson@chiark.greenend.org.uk>
On 11/04/2016 08:13 PM, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
> ---
> Documentation/git-check-ref-format.txt | 10 ++++++++--
> builtin/check-ref-format.c | 34 +++++++++++++++++++++++++++++++---
> 2 files changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
> index e9a2657..5a213ce 100644
> --- a/Documentation/git-check-ref-format.txt
> +++ b/Documentation/git-check-ref-format.txt
> @@ -10,8 +10,9 @@ SYNOPSIS
> [verse]
> 'git check-ref-format' [--report-errors] [--normalize]
> [--[no-]allow-onelevel] [--refspec-pattern]
> - <refname>
> -'git check-ref-format' [--report-errors] --branch <branchname-shorthand>
> + <refname> | --stdin
> +'git check-ref-format' [--report-errors] --branch
> + <branchname-shorthand> | --stdin
>
> DESCRIPTION
> -----------
> @@ -109,6 +110,11 @@ OPTIONS
> If any ref does not check OK, print a message to stderr.
> (By default, git check-ref-format is silent.)
>
> +--stdin::
> + Instead of checking on ref supplied on the command line,
> + read refs, one per line, from stdin. The exit status is
> + 0 if all the refs were OK.
> +
>
> EXAMPLES
> --------
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index 559d5c2..87f52fa 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -76,6 +76,7 @@ static int check_one_ref_format(const char *refname)
> int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> {
> int i;
> + int use_stdin = 0;
>
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage(builtin_check_ref_format_usage);
> @@ -93,6 +94,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> check_branch = 1;
> else if (!strcmp(argv[i], "--report-errors"))
> report_errors = 1;
> + else if (!strcmp(argv[i], "--stdin"))
> + use_stdin = 1;
> else
> usage(builtin_check_ref_format_usage);
> }
> @@ -100,8 +103,33 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> if (check_branch && (flags || normalize))
> usage(builtin_check_ref_format_usage);
>
> - if (! (i == argc - 1))
> - usage(builtin_check_ref_format_usage);
> + if (!use_stdin) {
> + if (! (i == argc - 1))
> + usage(builtin_check_ref_format_usage);
Given the changes that you made to support `--stdin`, it would be pretty
easy to support multiple command line arguments, now, too. (But this
needn't be part of your patch series.)
> +
> + return check_one_ref_format(argv[i]);
> + } else {
> + char buffer[2048];
> + int worst = 0;
>
> - return check_one_ref_format(argv[i]);
> + if (! (i == argc))
> + usage(builtin_check_ref_format_usage);
> +
> + while (fgets(buffer, sizeof(buffer), stdin)) {
`strbuf_getline()` would make this a lot easier and also eliminate the
need to specify a buffer size.
> + char *newline = strchr(buffer, '\n');
> + if (!newline) {
> + fprintf(stderr, "%s --stdin: missing final newline or line too long\n", *argv);
> + exit(127);
> + }
> + *newline = 0;
> + int got = check_one_ref_format(buffer);
> + if (got > worst)
> + worst = got;
> + }
> + if (!feof(stdin)) {
> + perror("reading from stdin");
> + exit(127);
> + }
> + return worst;
> + }
> }
>
Michael
^ permalink raw reply
* Re: [PATCH 0/5] git check-ref-format --stdin --report-errors
From: Michael Haggerty @ 2016-12-19 11:29 UTC (permalink / raw)
To: Ian Jackson, git
In-Reply-To: <20161104191358.28812-1-ijackson@chiark.greenend.org.uk>
On 11/04/2016 08:13 PM, Ian Jackson wrote:
> I wanted to be able to syntax check lots of proposed refs quickly
> (please don't ask why - it's complicated!)
>
> So I added a --stdin option to git-check-ref-format. Also it has
> --report-errors now too so you can get some kind of useful error
> message if it complains.
>
> It's still not really a good batch mode but it's good enough for my
> use case. To improve it would involve a new command line option to
> offer a suitable stdout output format.
>
> There are three small refactoring patches and the two patches with new
> options and corresponding docs.
>
> Thanks for your attention.
>
> FYI I am not likely to need this again in the near future: it's a
> one-off use case. So my effort for rework is probably limited. I
> thought I'd share what I'd done in what I hope is a useful form,
> anyway.
Thanks for your patches. I left some comments about the individual patches.
I don't know whether this feature will be popular, but it's not a lot of
code to add it, so it would be OK with me.
Especially given that the output is not especially machine-readable, it
might be more consistent with other commands to call the new feature
`--verbose` rather than `--report-errors`.
If it is thought likely that scripts will want to leave a pipe open to
this command and feed it one query at a time, then it would be helpful
to flush stdout after each reference's result is written. If the
opposite use case is common (mass processing of refnames), we could
always add a `--buffer` option like the one that `git cat-file --batch` has.
Michael
^ permalink raw reply
* Re: [PATCH v2 10/21] read-cache: regenerate shared index if necessary
From: Duy Nguyen @ 2016-12-19 11:48 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161217145547.11748-11-chriscool@tuxfamily.org>
On Sat, Dec 17, 2016 at 03:55:36PM +0100, Christian Couder wrote:
> +static const int default_max_percent_split_change = 20;
> +
> +static int too_many_not_shared_entries(struct index_state *istate)
> +{
> + int i, not_shared = 0;
> + int max_split = git_config_get_max_percent_split_change();
> +
> + switch (max_split) {
> + case -1:
> + /* not or badly configured: use the default value */
> + max_split = default_max_percent_split_change;
> + break;
> + case 0:
> + return 1; /* 0% means always write a new shared index */
> + case 100:
> + return 0; /* 100% means never write a new shared index */
I wonder if we really need to special case these here. If I read it
correctly, the expression at the end of this function will return 1
when max_split is 0, and 0 when max_split is 100 (not counting the
case when cache_nr is zero).
Perhaps it's good for documentation purpose. Though I find it hard to
see a use case for max_split == 0. Always creating a new shared index
sounds crazy.
> + default:
> + ; /* do nothing: just use the configured value */
> + }
> +
> + /* Count not shared entries */
> + for (i = 0; i < istate->cache_nr; i++) {
> + struct cache_entry *ce = istate->cache[i];
> + if (!ce->index)
> + not_shared++;
> + }
> +
> + return istate->cache_nr * max_split < not_shared * 100;
> +}
^ permalink raw reply
* Re: [PATCH v2 16/21] read-cache: unlink old sharedindex files
From: Duy Nguyen @ 2016-12-19 11:58 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161217145547.11748-17-chriscool@tuxfamily.org>
On Sat, Dec 17, 2016 at 03:55:42PM +0100, Christian Couder wrote:
> +static int clean_shared_index_files(const char *current_hex)
> +{
> + struct dirent *de;
> + DIR *dir = opendir(get_git_dir());
> +
> + if (!dir)
> + return error_errno(_("unable to open git dir: %s"), get_git_dir());
> +
> + while ((de = readdir(dir)) != NULL) {
> + const char *sha1_hex;
> + if (!skip_prefix(de->d_name, "sharedindex.", &sha1_hex))
> + continue;
> + if (!strcmp(sha1_hex, current_hex))
fspathcmp since we're dealing fs paths?
In theory we should be ok using strcmp, even on Windows because case
is preserved, I think. It's only a problem when we write path 'abc'
down and read 'ABC' back.
Oh well.. your call because if you go with fspathcmp, skip_prefix can't
be used either. A lot more changes for a very rare case.
> + continue;
> + if (can_delete_shared_index(sha1_hex) > 0 &&
Probably shorter to pass full d->name here so you don't have to do
another git_path() in can_delete_
> + unlink(git_path("%s", de->d_name)))
> + error_errno(_("unable to unlink: %s"), git_path("%s", de->d_name));
> + }
> + closedir(dir);
> +
> + return 0;
> +}
^ permalink raw reply
* Re: [PATCH v2 00/21] Add configuration options for split-index
From: Duy Nguyen @ 2016-12-19 12:02 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161217145547.11748-1-chriscool@tuxfamily.org>
On Sat, Dec 17, 2016 at 03:55:26PM +0100, Christian Couder wrote:
> Goal
> ~~~~
>
> We want to make it possible to use the split-index feature
> automatically by just setting a new "core.splitIndex" configuration
> variable to true.
>
> This can be valuable as split-index can help significantly speed up
> `git rebase` especially along with the work to libify `git apply`
> that has been merged to master
> (see https://github.com/git/git/commit/81358dc238372793b1590efa149cc1581d1fbd98)
> and is now in v2.11.
I've read through the series (*) and I think it looks good, just a few
minor comments here and there.
(*) guiltily admit that I only skimmed through tests, not giving them
as much attention as I should have
--
Duy
^ permalink raw reply
* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
From: Duy Nguyen @ 2016-12-19 12:07 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161214151009.4wdzjb44f6aki5ug@sigill.intra.peff.net>
On Wed, Dec 14, 2016 at 10:10:10AM -0500, Jeff King wrote:
> diff --git a/parse-options.c b/parse-options.c
> index 312a85dbd..4fbe924a5 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -661,7 +661,7 @@ void NORETURN usage_msg_opt(const char *msg,
> const char * const *usagestr,
> const struct option *options)
> {
> - fprintf(stderr, "%s\n\n", msg);
> + fprintf(stderr, "fatal: %s\n\n", msg);
Your commit message does not make clear if you want this "fatal" to be
grep-able (by scripts) or not. If not, please _() the string. Maybe
this to reduce work for translators
/* TRANSLATORS: this is the prefix before usage error */
fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);
> usage_with_options(usagestr, options);
> }
>
> --
> 2.11.0.341.g202cd3142
^ permalink raw reply
* Re: [PATCH 2/5] check-ref-format: Refactor to make --branch code more common
From: Ian Jackson @ 2016-12-19 11:54 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <e93ee78a-aa5e-27f6-9703-6efa385f487b@alum.mit.edu>
Michael Haggerty writes ("Re: [PATCH 2/5] check-ref-format: Refactor to make --branch code more common"):
> On 11/04/2016 08:13 PM, Ian Jackson wrote:
> > static int normalize = 0;
> > +static int check_branch = 0;
> > static int flags = 0;
> >
> > static int check_one_ref_format(const char *refname)
> > {
> > + int got;
>
> `got` is an unusual name for this variable, and I don't really
> understand what the word means in this context. Is there a reason not to
> use the more usual `err`?
I have no real opinion about the name of this variable. `err' is a
fine name too.
> > + if (check_branch && (flags || normalize))
>
> Is there a reason not to allow `--normalize` with `--branch`?
> (Currently, `git check-ref-format --branch` *does* allow input like
> `refs/heads/foo`.)
It was like that when I found it :-). I wasn't sure why this
restriction was there so I left it alone.
Looking at it again: AFAICT from the documentation --branch is a
completely different mode. The effect of --normalize is not to do
additional work, but simply to produce additional output. It really
means --print-normalized. --branch already prints output, but AFAICT
it does not collapse slashes. This seems like a confusing collection
of options. But, sorting that out is beyond the scope of what I was
trying to do.
In my series I have at least managed not to make any of this any
worse, I think: the --stdin option I introduce applies to both modes
equally, and doesn't make future improvements to the conflict between
--branch and --normalize any harder.
(In _this_ patch, certainly, allowing --normalize with --branch would
be wrong, since _this_ patch is just refactoring.)
Thanks,
Ian.
--
Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own.
If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
^ permalink raw reply
* Re: [PATCH v2 02/34] sequencer (rebase -i): implement the 'noop' command
From: Johannes Schindelin @ 2016-12-19 12:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Torvalds, Git Mailing List, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <xmqqtwa7ze9i.fsf@gitster.mtv.corp.google.com>
Hi,
On Tue, 13 Dec 2016, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >>
> >>> +/*
> >>> + * Note that ordering matters in this enum. Not only must it match the mapping
> >>> + * below, it is also divided into several sections that matter. When adding
> >>> + * new commands, make sure you add it in the right section.
> >>> + */
> >>
> >> Good thinking.
Not my thinking... This was in direct response to a suggestion by Dennis
Kaarsemaker, I cannot take credit for the idea.
> Makes me wish C were a better language, though ;-)
> >
> > Do this:
> >
> > static const char *todo_command_strings[] = {
> > [TODO_PICK] = "pick",
> > [TODO_REVERT] = "revert",
> > [TODO_NOOP] = "noop:,
> > };
> >
> > which makes the array be order-independent. You still need to make
> > sure you fill in all the entries, of course, but it tends to avoid at
> > least one gotcha, and it makes it more obvious how the two are tied
> > together.
>
> Yes, I know. But I do not think the variant of C we stick to is not
> new enough to have that.
Let me try to express this without double negation: our coding guidelines
state very explicitly that we do not use C99 initializers, and it also
explains why: we want to support a broader range of compilers. For
details, see:
https://github.com/git/git/blob/v2.11.0/Documentation/CodingGuidelines#L179-L181
TBH I briefly considered going the same route as I did for the fsck
warnings (taking a lot of heat for the ugliness):
https://github.com/git/git/blob/v2.11.0/fsck.c#L17-L85
It would have looked somewhat like this:
#define FOREACH_TODO(FUNC) \
FUNC(PICK, 'p', "pick") \
FUNC(REVERT, 0, "revert") \
FUNC(EDIT, 'e', "edit") \
...
FUNC(COMMENT, 0, NULL)
#define TODO(id, short, long) TODO_##id,
enum todo_command {
FOREACH_TODO(TODO)
TODO_MAX
};
#undef TODO
#define TODO(id, short, long) { short, long },
static struct {
char c;
const char *str;
} todo_command_info[] = {
FOREACH_TODO(TODO)
{ 0, NULL }
};
#undef TODO
I have to admit that even I prefer the version I provided in my
contribution over the "fsck method" outlined above.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v2 00/34] Teach the sequencer to act as rebase -i's backend
From: Johannes Schindelin @ 2016-12-19 13:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <xmqqa8bz39aw.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 13 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > Apart from mostly cosmetic patches (and the occasional odd bug that I
> > fixed promptly), I used these patches since mid May to perform all of my
> > interactive rebases. In mid June, I had the idea to teach rebase -i to
> > run *both* scripted rebase and rebase--helper and to cross-validate the
> > results. This slowed down all my interactive rebases since, but helped
> > me catch three rather obscure bugs (e.g. that git commit --fixup unfolds
> > long onelines and rebase -i still finds the correct original commit).
> > ...
> > Please note that the interdiff vs v1 is only of limited use: too many
> > things changed in the meantime, in particular the prepare-sequencer
> > branch that went through a couple of iterations before it found its way
> > into git.git's master branch. So please take the interdiff with a
> > mountain range of salt.
> > ...
> > Changes since v1:
> > ...
> > - removed the beautiful ordinal logic (to print out "1st", "2nd", "3rd"
> > etc) and made things consistent with the current `rebase -i`.
>
> It was removed because it was too Anglo-centric and unusable in i18n
> context, no?
Yes, but I remember putting in a lot of time to try to come up with the
most elegant way to convert a number into an English ordinal in a shell
function. That's where all that wistfulness came from.
> Judging from the list above, interdiff are pretty much all cosmetic
> and that is why you say it is only of limited use, I guess.
No, I said that it is only of limited use because I had to fudge things to
come up with an interdiff. I had to fudge things because there is no
interdiff: it would require the previous iteration of the patch series to
apply cleanly to the current `master`, which it does not. So I rebased the
patches and left as much intact as possible, which means that the rebased
changes do not even compile because they were based on a previous
iteration of the prepare-sequencer patch series that did not make it into
`master` without substantial changes.
> ... goes and reads the remainder and finds that these were
> ... all minor changes, mostly cosmetic, with a helper function
> ... refactored out or two and things of that nature.
>
> It is actually a good thing. We do not want to see it deviate too
> drastically from what you have been testing for some months.
Well, that ship has sailed. Neither of the patch series
"require-clean-work-tree", "libify-sequencer" and "prepare-sequencer" made
it into `master` without substantial deviations from the code I had been
testing and improving for half a year. I did not expect the code to be
accepted unchanged, anyways.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/5] check-ref-format: Refactor out check_one_ref_format
From: Ian Jackson @ 2016-12-19 13:19 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <3e277bb8-bd1f-0d8c-47a7-9673ad711bce@alum.mit.edu>
Michael Haggerty writes ("Re: [PATCH 1/5] check-ref-format: Refactor out check_one_ref_format"):
> On 11/04/2016 08:13 PM, Ian Jackson wrote:
> > +static int check_one_ref_format(const char *refname)
...
> This function needs to `return 0` if it gets to the end.
Indeed it does. I'm kind of surprised my compiler didn't spot that.
Thanks for the careful review!
Regards,
Ian.
--
Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own.
If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
^ permalink raw reply
* Re: [PATCH v2 01/34] sequencer: support a new action: 'interactive rebase'
From: Johannes Schindelin @ 2016-12-19 13:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <xmqqk2b31sfs.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 13 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > @@ -395,7 +414,10 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> >
> > if (active_cache_changed &&
> > write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
> > - /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> > + /*
> > + * TRANSLATORS: %s will be "revert", "cherry-pick" or
> > + * "rebase -i".
> > + */
>
> IIRC, the "TRANSLATORS:" comment has to deviate from our coding
> style due to tool limitation and has to be done like this:
>
> > + /* TRANSLATORS: %s will be "revert", "cherry-pick" or
> > + * "rebase -i".
> > + */
Aargh. I had fixed that in another patch series already, but failed to do
so in this here patch series. Sorry.
Fixed.
> > @@ -1204,6 +1226,9 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
> > const char *todo_path = get_todo_path(opts);
> > int next = todo_list->current, offset, fd;
> >
> > + if (is_rebase_i(opts))
> > + next++;
> > +
>
> This is because...? Everybody else counts 0-based while rebase-i
> counts from 1 or something?
No. Since its conception, edit-patch-series.sh (the artist now known as
rebase -i) processed each line of the "todo script" by writing a new
"todo" file, skipping the first line, and appending it to the "done" file.
The sequencer chooses to write the new "insn sheet" after a successful
operation.
This is not an option for rebase -i, as it has many error paths and it was
simply impossible to express "save the todo script in case of failure" in
shell script.
I added a comment to clarify why the current command is not written to
git-rebase-todo.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v2 02/34] sequencer (rebase -i): implement the 'noop' command
From: Johannes Schindelin @ 2016-12-19 13:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <xmqqfulr1s5z.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 13 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > @@ -1292,7 +1316,12 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
> > struct todo_item *item = todo_list->items + todo_list->current;
> > if (save_todo(todo_list, opts))
> > return -1;
> > - res = do_pick_commit(item->command, item->commit, opts);
> > + if (item->command <= TODO_REVERT)
> > + res = do_pick_commit(item->command, item->commit,
> > + opts);
> > + else if (item->command != TODO_NOOP)
> > + return error(_("unknown command %d"), item->command);
>
> I wonder if making this a switch() statement is easier to read in
> the longer run. The only thing at this point we are gaining by "not
> only mapping and enum must match, the orders matter" is so that this
> codepath can do the same thing for PICK and REVERT, but these two
> would become more and more minority as we learn more words.
I doubt that this is easier to read. There are essentially three
categories we are handling: exec, comments, and everything else. IMO the
current code is the easiest to understand.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
From: Jeff King @ 2016-12-19 13:41 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <20161219120719.GF24125@ash>
On Mon, Dec 19, 2016 at 07:07:19PM +0700, Duy Nguyen wrote:
> On Wed, Dec 14, 2016 at 10:10:10AM -0500, Jeff King wrote:
> > diff --git a/parse-options.c b/parse-options.c
> > index 312a85dbd..4fbe924a5 100644
> > --- a/parse-options.c
> > +++ b/parse-options.c
> > @@ -661,7 +661,7 @@ void NORETURN usage_msg_opt(const char *msg,
> > const char * const *usagestr,
> > const struct option *options)
> > {
> > - fprintf(stderr, "%s\n\n", msg);
> > + fprintf(stderr, "fatal: %s\n\n", msg);
>
> Your commit message does not make clear if you want this "fatal" to be
> grep-able (by scripts) or not. If not, please _() the string. Maybe
> this to reduce work for translators
>
> /* TRANSLATORS: this is the prefix before usage error */
> fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);
I don't think we translate any of our "fatal:", "error:", etc, do we?
It certainly doesn't look like it in usage.c.
-Peff
^ permalink raw reply
* Re: [PATCH 5/5] check-ref-format: New --stdin option
From: Michael Haggerty @ 2016-12-19 13:43 UTC (permalink / raw)
To: Ian Jackson, git
In-Reply-To: <20161104191358.28812-6-ijackson@chiark.greenend.org.uk>
On 11/04/2016 08:13 PM, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
> ---
> Documentation/git-check-ref-format.txt | 10 ++++++++--
> builtin/check-ref-format.c | 34 +++++++++++++++++++++++++++++++---
> 2 files changed, 39 insertions(+), 5 deletions(-)
>
> [...]
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index 559d5c2..87f52fa 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -76,6 +76,7 @@ static int check_one_ref_format(const char *refname)
> int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> {
> int i;
> + int use_stdin = 0;
>
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage(builtin_check_ref_format_usage);
> @@ -93,6 +94,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> check_branch = 1;
> else if (!strcmp(argv[i], "--report-errors"))
> report_errors = 1;
> + else if (!strcmp(argv[i], "--stdin"))
> + use_stdin = 1;
> else
> usage(builtin_check_ref_format_usage);
> }
> @@ -100,8 +103,33 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> if (check_branch && (flags || normalize))
> usage(builtin_check_ref_format_usage);
>
> - if (! (i == argc - 1))
> - usage(builtin_check_ref_format_usage);
> + if (!use_stdin) {
> + if (! (i == argc - 1))
> + usage(builtin_check_ref_format_usage);
> +
> + return check_one_ref_format(argv[i]);
> + } else {
> + char buffer[2048];
> + int worst = 0;
>
> - return check_one_ref_format(argv[i]);
> + if (! (i == argc))
> + usage(builtin_check_ref_format_usage);
> +
> + while (fgets(buffer, sizeof(buffer), stdin)) {
> + char *newline = strchr(buffer, '\n');
> + if (!newline) {
> + fprintf(stderr, "%s --stdin: missing final newline or line too long\n", *argv);
> + exit(127);
> + }
> + *newline = 0;
> + int got = check_one_ref_format(buffer);
Another minor point: project policy is not to mix declarations and code.
Please declare `got` at the top of the block.
> + if (got > worst)
> + worst = got;
> + }
> + if (!feof(stdin)) {
> + perror("reading from stdin");
> + exit(127);
> + }
> + return worst;
> + }
> }
>
Michael
^ permalink raw reply
* Re: [PATCH v2 03/34] sequencer (rebase -i): implement the 'edit' command
From: Johannes Schindelin @ 2016-12-19 13:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <xmqqbmwf1pqd.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 13 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > @@ -43,6 +44,20 @@ static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
> > */
> > static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
> > /*
>
> It is minor, but please have a blank line to separate the logical
> blocks.
Ah, but where to draw the line? These comments in front of some of the
rebase_path_* functions clarify the particular role of the corresponding
paths, but all of those functions belong to the same block of
rebase_path_* functions. That latter circumstance is what made me decide
to *not* insert blank lines here, but only a blank line before that block
(to separate from the sequencer path functions) and after that block (to
separate from the rest of the code).
> If you have "comment for thing A" before "thing A", then having a blank
> after that before "comment for thing B" and "thing B" that follow would
> help. Otherwise "thing A" immediately followed by "comment for thing B"
> are (mis)read together, leading to nonsense.
In this case, I think it is quite obvious that the comments belong to the
immediately following line, and that all of the path functions are part of
a bigger block.
> > + * When an "edit" rebase command is being processed, the SHA1 of the
> > + * commit to be edited is recorded in this file. When "git rebase
> > + * --continue" is executed, if there are any staged changes then they
> > + * will be amended to the HEAD commit, but only provided the HEAD
> > + * commit is still the commit to be edited. When any other rebase
> > + * command is processed, this file is deleted.
> > + */
> > +static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
> > +/*
> > + * When we stop at a given patch via the "edit" command, this file contains
> > + * the long commit name of the corresponding patch.
>
> If you abbreviate an object name to 38-hex that is still long but
> that is not full; if you meant full 40-hex, better spell it out as
> "full"---that conveys useful information to programmers (e.g. they
> can just use get_sha1_hex()).
>
> But I think you are writing short_commit_name() to it? So perhaps
> "an abbreviated commit object name"?
Fixed.
> > @@ -1301,9 +1318,87 @@ static int save_opts(struct replay_opts *opts)
> > return res;
> > }
> >
> > +static int make_patch(struct commit *commit, struct replay_opts *opts)
> > +{
> > + struct strbuf buf = STRBUF_INIT;
> > + struct rev_info log_tree_opt;
> > + const char *commit_buffer = get_commit_buffer(commit, NULL), *subject, *p;
> > + int res = 0;
> > +
> > + p = short_commit_name(commit);
> > + if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
> > + return -1;
> > +
> > + strbuf_addf(&buf, "%s/patch", get_dir(opts));
> > + memset(&log_tree_opt, 0, sizeof(log_tree_opt));
> > + init_revisions(&log_tree_opt, NULL);
> > + log_tree_opt.abbrev = 0;
> > + log_tree_opt.diff = 1;
> > + log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
> > + log_tree_opt.disable_stdin = 1;
> > + log_tree_opt.no_commit_id = 1;
> > + log_tree_opt.diffopt.file = fopen(buf.buf, "w");
> > + log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
> > + if (!log_tree_opt.diffopt.file)
> > + res |= error_errno(_("could not open '%s'"), buf.buf);
> > + else {
> > + res |= log_tree_commit(&log_tree_opt, commit);
> > + fclose(log_tree_opt.diffopt.file);
> > + }
> > + strbuf_reset(&buf);
> > + strbuf_addf(&buf, "%s/message", get_dir(opts));
> > + if (!file_exists(buf.buf)) {
> > + find_commit_subject(commit_buffer, &subject);
> > + res |= write_message(subject, strlen(subject), buf.buf, 1);
> > + unuse_commit_buffer(commit, commit_buffer);
> > + }
> > + strbuf_release(&buf);
> > +
> > + return res;
> > +}
>
> OK. This seems to match what scripted make_patch does in a handful
> of lines. We probably should have given you a helper to reduce
> boilerplate that sets up log_tree_opt so that this function does not
> have to be this long, but that is a separate topic.
>
> Does it matter output_format is set to FORMAT_PATCH here, though?
> With --no-commit-id set, I suspect there is no log message or
> authorship information given to the output.
>
> As you are only interested in seeing the patch/diff in this file and
> the log is stored in a separate "message" file, as long as "patch"
> file gets the patch correctly, it is not a problem, but it just
> looked strange to see FORMAT_PATCH there.
I am indifferent as to FORMAT_PATCH. It is there simply as a direct
translation of the `diff-tree -p` command in
https://github.com/git/git/blob/v2.11.0/git-rebase--interactive.sh#L188
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
From: Duy Nguyen @ 2016-12-19 13:53 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161219134148.vxa4fz3jw2i23lfm@sigill.intra.peff.net>
On Mon, Dec 19, 2016 at 8:41 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 19, 2016 at 07:07:19PM +0700, Duy Nguyen wrote:
>
>> On Wed, Dec 14, 2016 at 10:10:10AM -0500, Jeff King wrote:
>> > diff --git a/parse-options.c b/parse-options.c
>> > index 312a85dbd..4fbe924a5 100644
>> > --- a/parse-options.c
>> > +++ b/parse-options.c
>> > @@ -661,7 +661,7 @@ void NORETURN usage_msg_opt(const char *msg,
>> > const char * const *usagestr,
>> > const struct option *options)
>> > {
>> > - fprintf(stderr, "%s\n\n", msg);
>> > + fprintf(stderr, "fatal: %s\n\n", msg);
>>
>> Your commit message does not make clear if you want this "fatal" to be
>> grep-able (by scripts) or not. If not, please _() the string. Maybe
>> this to reduce work for translators
>>
>> /* TRANSLATORS: this is the prefix before usage error */
>> fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);
>
> I don't think we translate any of our "fatal:", "error:", etc, do we?
> It certainly doesn't look like it in usage.c.
I know. But those existed before the l10n starts, some of those belong
to plumbing messages. This one is new.
--
Duy
^ permalink raw reply
* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
From: Jeff King @ 2016-12-19 14:05 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8BgfyDTUmNh_PvoVcz2q92eNTRZy5myegTdi8mu6imVUQ@mail.gmail.com>
On Mon, Dec 19, 2016 at 08:53:30PM +0700, Duy Nguyen wrote:
> >> Your commit message does not make clear if you want this "fatal" to be
> >> grep-able (by scripts) or not. If not, please _() the string. Maybe
> >> this to reduce work for translators
> >>
> >> /* TRANSLATORS: this is the prefix before usage error */
> >> fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);
> >
> > I don't think we translate any of our "fatal:", "error:", etc, do we?
> > It certainly doesn't look like it in usage.c.
>
> I know. But those existed before the l10n starts, some of those belong
> to plumbing messages. This one is new.
We add lots of new messages which are themselves translated, and they
still get untranslated prefixes. It seems like consistency is more
important than translating this one spot. But then, I do not use a
translated git myself, so I would not see the difference either way.
-Peff
^ permalink raw reply
* Re: [PATCH v2 04/34] sequencer (rebase -i): implement the 'exec' command
From: Johannes Schindelin @ 2016-12-19 14:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <xmqq7f731pis.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 13 Dec 2016, Junio C Hamano wrote:
> Please have "else if" on the same line as "}" that closes the
> "if (...) {" in the same if/else if/else cascade.
Fixed. This affected quite a few more patches than just this one, and
added two more to this already large patch series.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v2 00/34] Teach the sequencer to act as rebase -i's backend
From: Johannes Schindelin @ 2016-12-19 14:09 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <9d399b83-92c6-44e9-8415-7695e690e8be@kdbg.org>
Hi Hannes,
On Wed, 14 Dec 2016, Johannes Sixt wrote:
> Am 13.12.2016 um 16:29 schrieb Johannes Schindelin:
> > base-commit: 8d7a455ed52e2a96debc080dfc011b6bb00db5d2
> > Published-As: https://github.com/dscho/git/releases/tag/sequencer-i-v2
> > Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-i-v2
>
> Thank you so much!
>
> I would appreciate if you could publish a branch that contains the end game so
> that I can test it, too. Currently I am still using
>
> git://github.com/dscho/git interactive-rebase (fca871a3cf4d)
That's the one. Unless I forget, I use the Git garden shears to update
'interactive-rebase' whenever I update 'sequencer-i', too.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
From: Jeff King @ 2016-12-19 14:32 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8AswJb1wzV+mcQ3HXrib3HRtVje7PPd9AWPE9Bi32_2sw@mail.gmail.com>
On Mon, Dec 19, 2016 at 09:30:09PM +0700, Duy Nguyen wrote:
> On Mon, Dec 19, 2016 at 9:05 PM, Jeff King <peff@peff.net> wrote:
> > On Mon, Dec 19, 2016 at 08:53:30PM +0700, Duy Nguyen wrote:
> >
> >> >> Your commit message does not make clear if you want this "fatal" to be
> >> >> grep-able (by scripts) or not. If not, please _() the string. Maybe
> >> >> this to reduce work for translators
> >> >>
> >> >> /* TRANSLATORS: this is the prefix before usage error */
> >> >> fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);
> >> >
> >> > I don't think we translate any of our "fatal:", "error:", etc, do we?
> >> > It certainly doesn't look like it in usage.c.
> >>
> >> I know. But those existed before the l10n starts, some of those belong
> >> to plumbing messages. This one is new.
> >
> > We add lots of new messages which are themselves translated, and they
> > still get untranslated prefixes. It seems like consistency is more
> > important than translating this one spot. But then, I do not use a
> > translated git myself, so I would not see the difference either way.
>
> I'm kinda used to the half English half Vietnamese messages after so
> many years (not just git). I do like the prefix translated as well.
> But I guess we could leave this a is for now. At least we know the
> scope of this message and will have easier time i18n-izing it when we
> do the same for die(), warning() and friends.
Yeah. It's a big enough change that at the very least it should go into
its own patch. I don't have a strong opinion myself, except that if we
want to leave plumbing messages completely grep-able, we probably need
to distinguish between different calls to die(), etc. Which sounds kind
of nasty.
I dunno. I _thought_ nobody was supposed to be grepping stderr, even for
plumbing commands. But maybe that does not match reality.
-Peff
^ permalink raw reply
* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
From: Duy Nguyen @ 2016-12-19 14:30 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161219140535.ehrbosgn32nl27ki@sigill.intra.peff.net>
On Mon, Dec 19, 2016 at 9:05 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 19, 2016 at 08:53:30PM +0700, Duy Nguyen wrote:
>
>> >> Your commit message does not make clear if you want this "fatal" to be
>> >> grep-able (by scripts) or not. If not, please _() the string. Maybe
>> >> this to reduce work for translators
>> >>
>> >> /* TRANSLATORS: this is the prefix before usage error */
>> >> fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);
>> >
>> > I don't think we translate any of our "fatal:", "error:", etc, do we?
>> > It certainly doesn't look like it in usage.c.
>>
>> I know. But those existed before the l10n starts, some of those belong
>> to plumbing messages. This one is new.
>
> We add lots of new messages which are themselves translated, and they
> still get untranslated prefixes. It seems like consistency is more
> important than translating this one spot. But then, I do not use a
> translated git myself, so I would not see the difference either way.
I'm kinda used to the half English half Vietnamese messages after so
many years (not just git). I do like the prefix translated as well.
But I guess we could leave this a is for now. At least we know the
scope of this message and will have easier time i18n-izing it when we
do the same for die(), warning() and friends.
--
Duy
^ 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