* Bug report: $program_name in error message
From: Josh Bleecher Snyder @ 2016-12-18 20:36 UTC (permalink / raw)
To: git
To reproduce, run 'git submodule' from within a bare repo. Result:
$ git submodule
fatal: $program_name cannot be used without a working tree.
Looks like the intent was for $program_name to be interpolated.
As an aside, I sent a message a few days ago about a segfault when
working with a filesystem with direct_io on, but it appears not to
have made it to the archives on marc.info. Am I perhaps still
greylisted?
Thanks,
Josh
^ permalink raw reply
* Re: Bug report: $program_name in error message
From: Stefan Beller @ 2016-12-18 21:34 UTC (permalink / raw)
To: Josh Bleecher Snyder; +Cc: git@vger.kernel.org
In-Reply-To: <CAFAcib8yauNRB6UbO8qS2_Ff4fDt-7mMsmgop8d1V0j=RoTBSA@mail.gmail.com>
On Sun, Dec 18, 2016 at 12:36 PM, Josh Bleecher Snyder
<josharian@gmail.com> wrote:
> To reproduce, run 'git submodule' from within a bare repo. Result:
>
> $ git submodule
> fatal: $program_name cannot be used without a working tree.
>
> Looks like the intent was for $program_name to be interpolated.
Which version of git do you use?
>
>
> As an aside, I sent a message a few days ago about a segfault when
> working with a filesystem with direct_io on, but it appears not to
> have made it to the archives on marc.info. Am I perhaps still
> greylisted?
Both emails show up in my mailbox (subscribed to the mailing list),
so I think that just nobody answered your first email as the answer
may be non trivial.
>
> Thanks,
> Josh
^ permalink raw reply
* Re: Bug report: $program_name in error message
From: Josh Bleecher Snyder @ 2016-12-18 21:44 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kZ=QK5s0_94+4GNs3M5oo49GLm-KkT5K=yZktxX8C4UCw@mail.gmail.com>
>> To reproduce, run 'git submodule' from within a bare repo. Result:
>>
>> $ git submodule
>> fatal: $program_name cannot be used without a working tree.
>>
>> Looks like the intent was for $program_name to be interpolated.
>
> Which version of git do you use?
$ git version
git version 2.11.0
>> As an aside, I sent a message a few days ago about a segfault when
>> working with a filesystem with direct_io on, but it appears not to
>> have made it to the archives on marc.info. Am I perhaps still
>> greylisted?
>
> Both emails show up in my mailbox (subscribed to the mailing list),
> so I think that just nobody answered your first email as the answer
> may be non trivial.
Thanks for the confirmation.
-josh
^ permalink raw reply
* Re: Suggestion for the "Did you mean this?" feature
From: Chris Packham @ 2016-12-19 0:48 UTC (permalink / raw)
To: Kaartic Sivaraam; +Cc: GIT
In-Reply-To: <1482063500.10858.1.camel@gmail.com>
On Mon, Dec 19, 2016 at 1:18 AM, Kaartic Sivaraam
<kaarticsivaraam91196@gmail.com> wrote:
> Hello all,
>
> I have found the "Did you mean this?" feature of git as a very good
> feature. I thought it would be even better if it took a step toward by
> asking for a prompt when there was only one alternative to the command
> that was entered.
>
> E.g.
>
>> unique@unique-pc:~$ git hepl
>> git: 'hepl' is not a git command. See 'git --help'.
>>
>> Did you mean this?
>> help
>> [yes/No] : y
>> usage: git [--version] [--help] [-C <path>] [-c name=value]
>> [--exec-path[=<path>]] [--html-path] [--man-path] [--info-
>> path]
>> ....
>
> This would make it even better for the user as it would avoid having to
> correct the mistake long commands that had only a single error
> (considering history feature is enabled).
>
> Is this is a good idea ?
This feature already exists (although it's not interactive). See
help.autoCorrect in the git-config man page. "git config
help.autoCorrect -1" should to the trick.
^ permalink raw reply
* Re: [PATCH] winansi_isatty(): fix when Git is used from CMD
From: Junio C Hamano @ 2016-12-19 1:12 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Pranit Bauva
In-Reply-To: <ffc6a7a0-4ae4-b755-0b09-5bcd7114a2e6@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> ..
> The new isatty() override implemented by cbb3f3c9b197 (mingw: intercept
> isatty() to handle /dev/null as Git expects it, 2016-12-11) does not
> take into account that _get_osfhandle() returns the handle visible by
> the C code, which is the pipe. But it actually wants to investigate the
> properties of the handle that is actually connected to the outside
> world. Fortunately, there is already winansi_get_osfhandle(), which
> returns exactly this handle. Use it.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> I was able to test the idea earlier than anticipated and it does work
> for me.
I'll queue this in 'pu' so that I won't forget, but this time I'll
make sure I won't act on it until I see you two agree on the right
way forward.
Thanks.
^ permalink raw reply
* [PATCH] fast-import: properly fanout notes when tree is imported
From: Mike Hommey @ 2016-12-19 2:12 UTC (permalink / raw)
To: git; +Cc: johan, gitster
In typical uses of fast-import, trees are inherited from a parent
commit. In that case, the tree_entry for the branch looks like:
.versions[1].sha1 = $some_sha1
.tree = <tree structure loaded from $some_sha1>
However, when trees are imported, rather than inherited, that is not the
case. One can import a tree with a filemodify command, replacing the
root tree object.
e.g.
"M 040000 $some_sha1 \n"
In this case, the tree_entry for the branch looks like:
.versions[1].sha1 = $some_sha1
.tree = NULL
When adding new notes with the notemodify command, do_change_note_fanout
is called to get a notes count, and to do so, it loops over the
tree_entry->tree, but doesn't do anything when the tree is NULL.
In the latter case above, it means do_change_note_fanout thinks the tree
contains no note, and new notes are added with no fanout.
Interestingly, do_change_note_fanout does check whether subdirectories
have a NULL .tree, in which case it uses load_tree(). Which means the
right behaviour happens when using the filemodify command to import
subdirectories.
This change makes do_change_note_fanount call load_tree() whenever the
tree_entry it is given has no tree loaded, making all cases handled
equally.
Signed-off-by: Mike Hommey <mh@glandium.org>
---
This is something I should have submitted a patch for a long time ago, back
when this was discussed in the thread starting from
https://www.spinics.net/lists/git/msg242426.html. The message most relevant to
this patch in the thread is https://www.spinics.net/lists/git/msg242448.html.
fast-import.c | 8 +++++---
t/t9301-fast-import-notes.sh | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index cb545d7df5..5e528b1999 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2220,13 +2220,17 @@ static uintmax_t do_change_note_fanout(
char *fullpath, unsigned int fullpath_len,
unsigned char fanout)
{
- struct tree_content *t = root->tree;
+ struct tree_content *t;
struct tree_entry *e, leaf;
unsigned int i, tmp_hex_sha1_len, tmp_fullpath_len;
uintmax_t num_notes = 0;
unsigned char sha1[20];
char realpath[60];
+ if (!root->tree)
+ load_tree(root);
+ t = root->tree;
+
for (i = 0; t && i < t->entry_count; i++) {
e = t->entries[i];
tmp_hex_sha1_len = hex_sha1_len + e->name->str_len;
@@ -2278,8 +2282,6 @@ static uintmax_t do_change_note_fanout(
leaf.tree);
} else if (S_ISDIR(e->versions[1].mode)) {
/* This is a subdir that may contain note entries */
- if (!e->tree)
- load_tree(e);
num_notes += do_change_note_fanout(orig_root, e,
hex_sha1, tmp_hex_sha1_len,
fullpath, tmp_fullpath_len, fanout);
diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index 83acf68bc3..b408b2b32d 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -483,6 +483,47 @@ test_expect_success 'verify that lots of notes trigger a fanout scheme' '
'
+# Create another notes tree from the one above
+cat >>input <<INPUT_END
+commit refs/heads/other_commits
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+commit #$(($num_commit + 1))
+COMMIT
+
+from refs/heads/many_commits
+M 644 inline file
+data <<EOF
+file contents in commit #$(($num_commit + 1))
+EOF
+
+commit refs/notes/other_notes
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+committing one more note on a tree imported from a previous notes tree
+COMMIT
+
+M 040000 $(git log --no-walk --format=%T refs/notes/many_notes)
+N inline :$(($num_commit + 1))
+data <<EOF
+note for commit #$(($num_commit + 1))
+EOF
+INPUT_END
+
+test_expect_success 'verify that importing a notes tree respects the fanout scheme' '
+ git fast-import <input &&
+
+ # None of the entries in the top-level notes tree should be a full SHA1
+ git ls-tree --name-only refs/notes/other_notes |
+ while read path
+ do
+ if test $(expr length "$path") -ge 40
+ then
+ return 1
+ fi
+ done
+'
+
cat >>expect_non-note1 << EOF
This is not a note, but rather a regular file residing in a notes tree
EOF
--
2.11.0.dirty
^ permalink raw reply related
* Re: [PATCHv8 6/6] submodule: add absorb-git-dir function
From: Duy Nguyen @ 2016-12-19 5:35 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, bmwill
In-Reply-To: <20161212190435.10358-7-sbeller@google.com>
On Mon, Dec 12, 2016 at 11:04:35AM -0800, Stefan Beller wrote:
> diff --git a/dir.c b/dir.c
> index e0efd3c2c3..d872cc1570 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2773,3 +2773,15 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
> free(work_tree);
> free(git_dir);
> }
> +
> +/*
> + * Migrate the git directory of the given path from old_git_dir to new_git_dir.
> + */
> +void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
> +{
> + if (rename(old_git_dir, new_git_dir) < 0)
> + die_errno(_("could not migrate git directory from '%s' to '%s'"),
> + old_git_dir, new_git_dir);
> +
> + connect_work_tree_and_git_dir(path, new_git_dir);
Should we worry about recovering (e.g. maybe move new_git_dir back to
old_git_dir) if this connect_work_tree_and_git_dir() fails?
Both write_file() and git_config_set_.. in this function may die(). In
such a case the repo is in broken state and the user needs pretty good
submodule understanding to recover from it, I think.
Recovering is not easy (nor entirely safe) either, though I suppose if
we keep original copies for modified files, then we could restore them
after moving the directory back and pray the UNIX gods that all
operations succeed.
--
Duy
^ permalink raw reply
* Re: [PATCH 1/5] check-ref-format: Refactor out check_one_ref_format
From: Michael Haggerty @ 2016-12-19 8:27 UTC (permalink / raw)
To: Ian Jackson, git
In-Reply-To: <20161104191358.28812-2-ijackson@chiark.greenend.org.uk>
On 11/04/2016 08:13 PM, Ian Jackson wrote:
> We are going to want to reuse this. No functional change right now.
>
> It currently has a hidden memory leak if --normalize is used.
>
> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
> ---
> builtin/check-ref-format.c | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index eac4994..4d56caa 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -48,12 +48,22 @@ static int check_ref_format_branch(const char *arg)
> return 0;
> }
>
> +static int normalize = 0;
> +static int flags = 0;
> +
> +static int check_one_ref_format(const char *refname)
> +{
> + if (normalize)
> + refname = collapse_slashes(refname);
> + if (check_refname_format(refname, flags))
> + return 1;
> + if (normalize)
> + printf("%s\n", refname);
This function needs to `return 0` if it gets to the end.
> +}
> +
> int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> {
> int i;
> - int normalize = 0;
> - int flags = 0;
> - const char *refname;
>
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage(builtin_check_ref_format_usage);
> @@ -76,13 +86,5 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> if (! (i == argc - 1))
> usage(builtin_check_ref_format_usage);
>
> - refname = argv[i];
> - if (normalize)
> - refname = collapse_slashes(refname);
> - if (check_refname_format(refname, flags))
> - return 1;
> - if (normalize)
> - printf("%s\n", refname);
> -
> - return 0;
> + return check_one_ref_format(argv[i]);
> }
>
Michael
^ permalink raw reply
* [PATCH] config.c: handle error case for fstat() calls
From: Nguyễn Thái Ngọc Duy @ 2016-12-19 9:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, josharian, Nguyễn Thái Ngọc Duy
In-Reply-To: <CAFAcib_cY8FeLFkW1=MfR+P7xoupGK9DFegNY5boExHSRppAmg@mail.gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Will this fix the problem I'm replying to? I don't know. I found this
while checking the code and it should be fixed regardless.
config.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 83fdecb..4973256 100644
--- a/config.c
+++ b/config.c
@@ -2194,7 +2194,12 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
goto out_free;
}
- fstat(in_fd, &st);
+ if (fstat(in_fd, &st) == -1) {
+ error_errno(_("fstat on %s failed"), config_filename);
+ ret = CONFIG_INVALID_FILE;
+ goto out_free;
+ }
+
contents_sz = xsize_t(st.st_size);
contents = xmmap_gently(NULL, contents_sz, PROT_READ,
MAP_PRIVATE, in_fd, 0);
@@ -2414,7 +2419,10 @@ int git_config_rename_section_in_file(const char *config_filename,
goto unlock_and_out;
}
- fstat(fileno(config_file), &st);
+ if (fstat(fileno(config_file), &st) == -1) {
+ ret = error_errno(_("fstat on %s failed"), config_filename);
+ goto out;
+ }
if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {
ret = error_errno("chmod on %s failed",
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* Re: [PATCH 2/5] check-ref-format: Refactor to make --branch code more common
From: Michael Haggerty @ 2016-12-19 11:07 UTC (permalink / raw)
To: Ian Jackson, git
In-Reply-To: <20161104191358.28812-3-ijackson@chiark.greenend.org.uk>
On 11/04/2016 08:13 PM, Ian Jackson wrote:
> We are going to want to permit other options with --branch.
>
> So, replace the special case with just an entry for --branch in the
> parser for ordinary options, and check for option compatibility at the
> end.
>
> No overall functional change.
>
> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
> ---
> builtin/check-ref-format.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index 4d56caa..f12c19c 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -49,13 +49,19 @@ static int check_ref_format_branch(const char *arg)
> }
>
> 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`?
> +
> if (normalize)
> refname = collapse_slashes(refname);
> - if (check_refname_format(refname, flags))
> + got = check_branch
> + ? check_ref_format_branch(refname)
> + : check_refname_format(refname, flags);
> + if (got)
> return 1;
> if (normalize)
> printf("%s\n", refname);
> @@ -68,9 +74,6 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage(builtin_check_ref_format_usage);
>
> - if (argc == 3 && !strcmp(argv[1], "--branch"))
> - return check_ref_format_branch(argv[2]);
> -
> for (i = 1; i < argc && argv[i][0] == '-'; i++) {
> if (!strcmp(argv[i], "--normalize") || !strcmp(argv[i], "--print"))
> normalize = 1;
> @@ -80,9 +83,15 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> flags &= ~REFNAME_ALLOW_ONELEVEL;
> else if (!strcmp(argv[i], "--refspec-pattern"))
> flags |= REFNAME_REFSPEC_PATTERN;
> + else if (!strcmp(argv[i], "--branch"))
> + check_branch = 1;
> else
> usage(builtin_check_ref_format_usage);
> }
> +
> + 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`.)
But note that simply allowing `--branch --normalize` without changing
`check_one_ref_format()` would mean generating *two* lines of output per
reference, so something else would have to change, too.
> + usage(builtin_check_ref_format_usage);
> +
> if (! (i == argc - 1))
> usage(builtin_check_ref_format_usage);
>
>
Michael
^ permalink raw reply
* 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
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