* Re: Fwd: Re: [git-multimail] License unknown (#1)
From: Andy Parkins @ 2013-02-12 15:11 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <511A04DA.7010809@alum.mit.edu>
On Tuesday 12 February 2013 09:01:14 Michael Haggerty wrote:
> I assume you are the Andy Parkins who originally submitted
> post-commit-email to the Git project...
I am indeed. Hello.
> I have derived another script from yours:
>
> https://github.com/mhagger/git-multimail
>
> I affixed the GPLv2 boilerplate to the code under the assumption that it
> inherited this license from the Git project. But afterwards I realized
> that I am not entirely confident that the GPLv2 applies to
> post-commit-email.
I'm sorry, that's entirely my fault. I had read somewhere in the git
submission policy that everything submitted was naturally under the same
license as git itself.
> Obviously the situation is even more complicated because other people
> have contributed patches to the script since your original submission.
> But it would nevertheless be very helpful if you would clarify your
> original intention, *especially* if your intention was to put the script
> under a license different than GPLv2.
My intention was to put my code under exactly the license the git itself is
under. I am delighted for you or anyone else to use or derive from it.
I'm also sorry I didn't respond to your first email, it accidentally got
sorted into spam.
If anyone wants some sort of legal declaration, I'm happy to give it.
> -------- Original Message --------
> Subject: Re: [git-multimail] License unknown (#1)
> Date: Sun, 27 Jan 2013 19:52:58 +0100
> From: Michael Haggerty <mhagger@alum.mit.edu>
> To: git discussion list <git@vger.kernel.org>, Andy Parkins
> <andyparkins@gmail.com>
> CC: mhagger/git-multimail
> <reply+i-10159725-60cb2c338c594bd09d77fe2f8d628aa55114a3f6-119718@reply.git
> hub.com>, Michiel Holtkamp <notifications@github.com>
>
> I have a question about the license of contrib/hooks/post-commit-email.
> I had assumed that since it is in the git project, which is GPLv2, and
> since it contains no contrary information, it would by implication also
> fall under GPLv2. But the file itself contains no explicit license
> information, and it is not clear to me that the "signed-off-by" line
> implies a particular license, either. (The signed-off-by *does* seem to
> imply that the source code is under some kind of open source license,
> but not which one.)
>
> If somebody can explain what license the code is under and how they come
> to that conclusion, I would be very grateful.
>
> And if Andy Parkins (the original author) is listening, please indicate
> whether you had any intent *other* than GPLv2.
>
> For anybody who is interested, the file was first committed in
> 4557e0de5b and has been modified by several authors since then.
>
> Given the pretty clear open-sourciness of the script, I don't think this
> has to be made into a big issue. But it would be nice to state the
> license explicitly for future users' information.
>
> Thanks,
> Michael
>
> On 01/27/2013 02:38 PM, Michiel Holtkamp wrote:
> > Actually, I'm not sure that it is GPLv2 for the original script. The
> > COPYING file in the main project declares the project as GPLv2, but it
> > also says that people contributing should make their preferences (for
> > licensing) known. Maybe we can assume it's GPLv2, (as the original
> > writer might have assumed it was GPLv2), but it's not explicitly stated
> > so I'm not sure (IANAL).
--
Dr Andy Parkins
Director
FussyLogic Ltd
tel: 0845 557 7645
web: www.fussylogic.co.uk
Company Registration No. 8198285 Registered in England and Wales
This email and any attachments to it may be confidential and are
intended solely for the use of the individual to whom it is addressed.
Any views or opinions expressed are solely those of the author and do
not necessarily represent those of FussyLogic Ltd.
If you are not the intended recipient of this email, you must neither
take any action based upon its contents, nor copy or show it to
anyone.
Please contact the sender if you believe you have received this email
in error.
^ permalink raw reply
* Re: [git-multimail] License unknown (#1)
From: Andy Parkins @ 2013-02-12 15:28 UTC (permalink / raw)
To: Michael Haggerty
Cc: git discussion list, mhagger/git-multimail, Michiel Holtkamp
In-Reply-To: <5105778A.1040401@alum.mit.edu>
On Sunday 27 January 2013 18:52:58 Michael Haggerty wrote:
> I have a question about the license of contrib/hooks/post-commit-email.
> I had assumed that since it is in the git project, which is GPLv2, and
> since it contains no contrary information, it would by implication also
> fall under GPLv2. But the file itself contains no explicit license
> information, and it is not clear to me that the "signed-off-by" line
> implies a particular license, either. (The signed-off-by *does* seem to
> imply that the source code is under some kind of open source license,
> but not which one.)
Keeping up with the git mailing list got a bit much, but my original filters
to sort messages from it into a (now hidden) folder where still in place.
Michael contacted me independently, which was enough of a prod to see this.
My apologies to everyone; I've been lax supporting the script -- I never
really expected it to be as widely used as it has been, I was always expecting
someone would come along and replace it so I'm pleased that Michael has done
so (although it's a little disheartening to read of it being called "hacky",
when I tried very hard to make it as clear and modular as I could).
> If somebody can explain what license the code is under and how they come
> to that conclusion, I would be very grateful.
>
> And if Andy Parkins (the original author) is listening, please indicate
> whether you had any intent *other* than GPLv2.
I intended it to be under the same license as Git. I had read in one of the
patch submission files (which I can't seem to find now) that all submissions
were considered part of git.
> For anybody who is interested, the file was first committed in
> 4557e0de5b and has been modified by several authors since then.
I haven't looked at it for a long time. I can't speak for the other authors,
but for any part of it that is still mine -- anyone is free to do anything
they wish with it.
> Given the pretty clear open-sourciness of the script, I don't think this
> has to be made into a big issue. But it would be nice to state the
> license explicitly for future users' information.
If an explicit declaration is needed, I am happy to give it. Let me know what
form this should take and I'll supply it.
Andy
--
Dr Andy Parkins
andyparkins@gmail.com
^ permalink raw reply
* Re: [RFC/PATCH] Replace filepattern with pathspec for consistency
From: Junio C Hamano @ 2013-02-12 16:31 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, Jiang Xin
In-Reply-To: <1360661084-8678-1-git-send-email-Matthieu.Moy@imag.fr>
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> pathspec is the most widely used term, and is the one defined in
> gitglossary.txt. <filepattern> was used only in the synopsys for git-add
> and git-commit, and in git-add.txt. Get rid of it.
>
> This patch is obtained with by running:
>
> perl -pi -e 's/filepattern/pathspec/' `git grep -l filepattern`
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> I'm a bit unsure about the changes to the .po files, but I guess doing
> the substitution there too does the right thing.
I am not sure if that is the right thing from the workflow point of
view, though.
The strings that are fed to _() would be updated with your patch,
but the replacement will stay to be "filepattern" translated to the
target language. Translators have to actively hunt for the messages
to update them. If you left the .po files untouched, they would
notice when git.pot is updated the next time and that will ensure
that the affected messages get translated, no?
> Documentation/git-add.txt | 12 ++++++------
> builtin/add.c | 2 +-
> builtin/commit.c | 4 ++--
> po/de.po | 6 +++---
> po/git.pot | 6 +++---
> po/sv.po | 6 +++---
> po/vi.po | 6 +++---
> po/zh_CN.po | 6 +++---
> 8 files changed, 24 insertions(+), 24 deletions(-)
For the above reason, I am inclined to take the first three and drop
the rest.
Thanks.
^ permalink raw reply
* Re: [RFC/PATCH] Replace filepattern with pathspec for consistency
From: Matthieu Moy @ 2013-02-12 17:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jiang Xin
In-Reply-To: <7vliatijej.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> The strings that are fed to _() would be updated with your patch,
> but the replacement will stay to be "filepattern" translated to the
> target language. Translators have to actively hunt for the messages
> to update them. If you left the .po files untouched, they would
> notice when git.pot is updated the next time and that will ensure
> that the affected messages get translated, no?
Right, and I'm not competent to say how filepattern was translated in
any of the languages. Let me know if you want a resend without the po/
part.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH v4] submodule: add 'deinit' command
From: Phil Hord @ 2013-02-12 17:11 UTC (permalink / raw)
To: Jens Lehmann
Cc: Git Mailing List, Junio C Hamano, Heiko Voigt, Michael J Gruber,
Marc Branchaud, W. Trevor King
In-Reply-To: <5112C6F6.4030607@web.de>
I haven't tried it yet, but I have some comments.
On Wed, Feb 6, 2013 at 4:11 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> With "git submodule init" the user is able to tell git he cares about one
> or more submodules and wants to have it populated on the next call to "git
> submodule update". But currently there is no easy way he could tell git he
> does not care about a submodule anymore and wants to get rid of his local
> work tree (except he knows a lot about submodule internals and removes the
> "submodule.$name.url" setting from .git/config together with the work tree
> himself).
>
> Help those users by providing a 'deinit' command. This removes the whole
> submodule.<name> section from .git/config either for the given
> submodule(s) or for all those which have been initialized if '.' is
> given. Fail if the current work tree contains modifications unless
> forced. Complain when for a submodule given on the command line the url
> setting can't be found in .git/config, but nonetheless don't fail.
>
> Add tests and link the man pages of "git submodule deinit" and "git rm"
> to assist the user in deciding whether removing or unregistering the
> submodule is the right thing to do for him.
>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
> ---
>
> Changes since v3:
> - Add deinit to the --force documentation of "git submodule"
> - Never remove submodules containing a .git dir, even when forced
> - Diagnostic output when "rm -rf" or "mkdir" fails
> - More test cases
>
>
> Documentation/git-rm.txt | 4 ++
> Documentation/git-submodule.txt | 18 +++++++-
> git-submodule.sh | 78 ++++++++++++++++++++++++++++++-
> t/t7400-submodule-basic.sh | 100 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 198 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
> index 92bac27..1d876c2 100644
> --- a/Documentation/git-rm.txt
> +++ b/Documentation/git-rm.txt
> @@ -149,6 +149,10 @@ files that aren't ignored are present in the submodules work tree.
> Ignored files are deemed expendable and won't stop a submodule's work
> tree from being removed.
>
> +If you only want to remove the local checkout of a submodule from your
> +work tree without committing the removal,
> +use linkgit:git-submodule[1] `deinit` instead.
> +
> EXAMPLES
> --------
> `git rm Documentation/\*.txt`::
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index a0c9df8..bc06159 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -13,6 +13,7 @@ SYNOPSIS
> [--reference <repository>] [--] <repository> [<path>]
> 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
> 'git submodule' [--quiet] init [--] [<path>...]
> +'git submodule' [--quiet] deinit [-f|--force] [--] <path>...
> 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch] [--rebase]
> [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
> 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
> @@ -134,6 +135,19 @@ init::
> the explicit 'init' step if you do not intend to customize
> any submodule locations.
>
> +deinit::
> + Unregister the given submodules, i.e. remove the whole
> + `submodule.$name` section from .git/config together with their work
> + tree. Further calls to `git submodule update`, `git submodule foreach`
> + and `git submodule sync` will skip any unregistered submodules until
> + they are initialized again, so use this command if you don't want to
> + have a local checkout of the submodule in your work tree anymore. If
> + you really want to remove a submodule from the repository and commit
> + that use linkgit:git-rm[1] instead.
> ++
> +If `--force` is specified, the submodule's work tree will be removed even if
> +it contains local modifications.
> +
> update::
> Update the registered submodules, i.e. clone missing submodules and
> checkout the commit specified in the index of the containing repository.
> @@ -213,8 +227,10 @@ OPTIONS
>
> -f::
> --force::
> - This option is only valid for add and update commands.
> + This option is only valid for add, deinit and update commands.
> When running add, allow adding an otherwise ignored submodule path.
> + When running deinit the submodule work trees will be removed even if
> + they contain local changes.
> When running update, throw away local changes in submodules when
> switching to a different commit; and always run a checkout operation
> in the submodule, even if the commit listed in the index of the
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 004c034..f1b552f 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -8,6 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
> USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]
> or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
> or: $dashless [--quiet] init [--] [<path>...]
> + or: $dashless [--quiet] deinit [-f|--force] [--] <path>...
> or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
> or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
> or: $dashless [--quiet] foreach [--recursive] <command>
> @@ -547,6 +548,81 @@ cmd_init()
> }
>
> #
> +# Unregister submodules from .git/config and remove their work tree
> +#
> +# $@ = requested paths (use '.' to deinit all submodules)
> +#
> +cmd_deinit()
> +{
> + # parse $args after "submodule ... init".
> + while test $# -ne 0
> + do
> + case "$1" in
> + -f|--force)
> + force=$1
> + ;;
> + -q|--quiet)
> + GIT_QUIET=1
> + ;;
> + --)
> + shift
> + break
> + ;;
> + -*)
> + usage
> + ;;
> + *)
> + break
> + ;;
> + esac
> + shift
> + done
> +
> + if test $# = 0
> + then
> + die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
> + fi
> +
> + module_list "$@" |
> + while read mode sha1 stage sm_path
> + do
> + die_if_unmatched "$mode"
> + name=$(module_name "$sm_path") || exit
> + url=$(git config submodule."$name".url)
> + if test -z "$url"
> + then
> + say "$(eval_gettext "No url found for submodule path '\$sm_path' in .git/config")"
Is it safe to shelter the user a little bit more from the git
internals here and say instead:
Submodule '\$sm_path' is not initialized.
Also, I think this code will show this message for each submodule on
'git submodule deinit .' But I think I would prefer to suppress it in
that case. If I have not explicitly stated which submodules to
deinit, then I do not think git should complain that some of them are
not initialized.
> + continue
> + fi
> +
> + # Remove the submodule work tree (unless the user already did it)
> + if test -d "$sm_path"
> + then
> + # Protect submodules containing a .git directory
> + if test -d "$sm_path/.git"
> + then
> + echo >&2 "$(eval_gettext "Submodule work tree $sm_path contains a .git directory")"
> + die "$(eval_gettext "(use 'rm -rf' if you really want to remove it including all of its history)")"
I expect this is the right thing to do for now. But I wonder if we
can also move $sm_path/.git to $GIT_DIR/modules/$sm_path in this case
(though I think I am not spelling this path correctly). Would that be
ok? What extra work is needed to relocate the .git dir like this?
> + fi
> +
> + if test -z "$force"
> + then
> + git rm -n "$sm_path" ||
> + die "$(eval_gettext "Submodule work tree $sm_path contains local modifications, use '-f' to discard them")"
Nit, grammar: use a semicolon instead of a comma.
> + fi
> + rm -rf "$sm_path" || say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")"
> + fi
> +
> + mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$sm_path'")"
> +
> + # Remove the whole section so we have a clean state when the
> + # user later decides to init this submodule again
> + git config --remove-section submodule."$name" &&
> + say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$sm_path'")"
> + done
> +}
> +
> +#
> # Update each submodule path to correct revision, using clone and checkout as needed
> #
> # $@ = requested paths (default to all)
> @@ -1157,7 +1233,7 @@ cmd_sync()
> while test $# != 0 && test -z "$command"
> do
> case "$1" in
> - add | foreach | init | update | status | summary | sync)
> + add | foreach | init | deinit | update | status | summary | sync)
> command=$1
> ;;
> -q|--quiet)
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 2683cba..f54a40d 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -757,4 +757,104 @@ test_expect_success 'submodule add with an existing name fails unless forced' '
> )
> '
>
> +test_expect_success 'set up a second submodule' '
> + git submodule add ./init2 example2 &&
> + git commit -m "submodle example2 added"
Nit: submodule is misspelled
> +'
> +
> +test_expect_success 'submodule deinit should remove the whole submodule section from .git/config' '
> + git config submodule.example.foo bar &&
> + git config submodule.example2.frotz nitfol &&
> + git submodule deinit init &&
> + test -z "$(git config submodule.example.url)" &&
> + test -z "$(git config submodule.example.foo)" &&
> + test -n "$(git config submodule.example2.url)" &&
> + test -n "$(git config submodule.example2.frotz)" &&
> + test -f example2/.git &&
> + rmdir init
> +'
> +
> +test_expect_success 'submodule deinit . deinits all initialized submodules' '
> + git submodule update --init &&
> + git config submodule.example.foo bar &&
> + git config submodule.example2.frotz nitfol &&
> + test_must_fail git submodule deinit &&
> + git submodule deinit . &&
> + test -z "$(git config submodule.example.url)" &&
> + test -z "$(git config submodule.example.foo)" &&
> + test -z "$(git config submodule.example2.url)" &&
> + test -z "$(git config submodule.example2.frotz)" &&
> + rmdir init example2
> +'
> +
> +test_expect_success 'submodule deinit deinits a submodule when its work tree is missing or empty' '
> + git submodule update --init &&
> + rm -rf init example2/* example2/.git &&
> + git submodule deinit init example2 &&
> + test -z "$(git config submodule.example.url)" &&
> + test -z "$(git config submodule.example2.url)" &&
> + rmdir init
> +'
> +
> +test_expect_success 'submodule deinit fails when the submodule contains modifications unless forced' '
> + git submodule update --init &&
> + echo X >>init/s &&
> + test_must_fail git submodule deinit init &&
> + test -n "$(git config submodule.example.url)" &&
> + test -f example2/.git &&
> + git submodule deinit -f init &&
> + test -z "$(git config submodule.example.url)" &&
> + rmdir init
> +'
> +
> +test_expect_success 'submodule deinit fails when the submodule contains untracked files unless forced' '
> + git submodule update --init &&
> + echo X >>init/untracked &&
> + test_must_fail git submodule deinit init &&
> + test -n "$(git config submodule.example.url)" &&
> + test -f example2/.git &&
> + git submodule deinit -f init &&
> + test -z "$(git config submodule.example.url)" &&
> + rmdir init
> +'
> +
> +test_expect_success 'submodule deinit fails when the submodule HEAD does not match unless forced' '
> + git submodule update --init &&
> + (
> + cd init &&
> + git checkout HEAD^
> + ) &&
> + test_must_fail git submodule deinit init &&
> + test -n "$(git config submodule.example.url)" &&
> + test -f example2/.git &&
> + git submodule deinit -f init &&
> + test -z "$(git config submodule.example.url)" &&
> + rmdir init
> +'
> +
> +test_expect_success 'submodule deinit complains but does not fail when used on an uninitialized submodule' '
> + git submodule update --init &&
> + git submodule deinit init >actual &&
> + test_i18ngrep "Submodule .example. (.*) unregistered for path .init" actual
> + git submodule deinit init >actual &&
> + test_i18ngrep "No url found for submodule path .init. in .git/config" actual &&
> + git submodule deinit . >actual &&
> + test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual
> + rmdir init example2
> +'
> +
> +test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '
> + git submodule update --init &&
> + (
> + cd init &&
> + rm .git &&
> + cp -R ../.git/modules/example .git &&
> + GIT_WORK_TREE=. git config --unset core.worktree
> + ) &&
> + test_must_fail git submodule deinit init &&
> + test_must_fail git submodule deinit -f init &&
> + test -d init/.git &&
> + test -n "$(git config submodule.example.url)"
> +'
> +
> test_done
> --
> 1.8.1.2.546.gea155c0
>
>
^ permalink raw reply
* Re: [PATCH v3 3/4] count-objects: report garbage files in pack directory too
From: Junio C Hamano @ 2013-02-12 17:23 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Ramsay Jones
In-Reply-To: <1360661277-17273-4-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> +/* A hook for count-objects to report invalid files in pack directory */
> +extern void (*report_garbage)(const char *desc, const char *path, int len, const char *name);
We may want to document the strange way the last three parameters
are used somewhere. e.g.
shows "path" (if "name" is NULL), or prepends "path" in
front of name (otherwise); only for the latter, "path" can
be a string that is not NUL-terminated but its length
specified with "len" and in that case a slash is inserted
between the path and the "name".
When described clearly, it sounds somewhat ugly and incoherent API,
even though it covers the immediate need X-<.
> + 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) - 5); /* ".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.
> + */
Do we need to even allow duplication? Why does prepare_packed_git_one()
below have to split the pathname into the base and extension in the first
place?
If you collect all pathnames that end with known extensions (".idx",
".pack" and ".keep") in the sorted "garbage" list as separate entries in
prepare_packed_git_one(), report_pack_garbage() can do the right thing
without trusting or iterating over packed_git list at all, I think. As
the entries are sorted, .pack, .idx and all other valid .ext are grouped
together for the same basename. If you see a group that have both .pack
and .idx, the group is good. Otherwise, everybody in the group is bad
(e.g. a lonely .pack file without .idx is an unusable garbage).
How about doing it something along this line, perhaps?
int i;
int beginning_of_this_name = -1;
int seen_bits = 0; /* 01 for .idx, 02 for .pack */
for (i = 0; i < list->nr; i++) {
if (beginning_of_this_name < 0)
beginning_of_this_name = i;
else if (list->items[i] and list->items[beginning_of_this_name]
share the same basename)
; /* keep scanning */
else {
/* one name ended at (i-1) */
if (seen_bits == 3)
; /* both .idx and .pack exist; good */
else
report_garbage_for_one_name(list, beginning_of_this_name, i,
seen_bits);
seen_bits = 0;
beginning_of_this_name = i;
}
if (list->items[i] is ".idx")
seen_bits |= 1;
if (list->items[i] is ".pack")
seen_bits |= 2;
}
if (0 <= beginning_of_this_name && seen_bits != 3)
report_garbages_for_one_name(list, beginning_of_this_name, list->nr, seen_bits);
with a helper function report_garbage_for_one_name() that would look like this:
report_garbage_for_one_name(...) {
int j;
const char *msg;
switch (seen_bits) {
case 0: msg = "no corresponding .idx nor .pack"; break;
case 1: msg = "no corresponding .pack"; break;
case 2: msg = "no corresponding .idx; break;
}
for (j = beginning_of_this_name; j < i; j++)
report_garbage(msg, list->items[j]);
}
For the above to work, prepare_packed_git_one() needs to retain only the
paths with known extensions in garbage list. "pack-deadbeef.unk" can and
should be reported as a garbage immediately when it is seen without being
placed in the list.
> @@ -1045,9 +1100,33 @@ static void prepare_packed_git_one(char *objdir, int local)
> if (!p)
> continue;
> install_packed_git(p);
> - }
> + } else if (!report_garbage) {
> + /*
> + * the rest of this if-chain requires
> + * report_garbage != NULL. Stop the chain if
> + * report_garbage is NULL.
> + */
> + ;
> + } else if (has_extension(de->d_name, ".pack")) {
> + struct string_list_item *item;
> + int n = strlen(path) - 5;
> + item = string_list_append_nodup(&garbage,
> + xstrndup(path, n));
> + item->util = ".pack";
> + continue;
> + } else if (has_extension(de->d_name, ".idx")) {
> + struct string_list_item *item;
> + int n = strlen(path) - 4;
> + item = string_list_append_nodup(&garbage,
> + xstrndup(path, n));
> + item->util = ".idx";
> + continue;
> + } else
> + report_garbage("garbage found", path, 0, NULL);
Hmm, where is a ".keep" file handled in this flow?
The structure of the if/else cascade is much nicer than the earlier
iterations, but wouldn't it be even more clear to do this?
if (is .idx file) {
... do that .idx thing ...
}
if (!report_garbage)
continue; /* it does not matter what the file is */
if (is .pack) {
... remember that we saw this .pack ...
} else if (is .idx) {
... remember that we saw this .idx ...
} else if (is .keep) {
... remember that we saw this .keep ...
} else {
... all else --- report as garbage immediately ...
}
^ permalink raw reply
* Re: [PATCH v2] rebase -i: respect core.commentchar
From: Junio C Hamano @ 2013-02-12 17:29 UTC (permalink / raw)
To: John Keeping; +Cc: git, Ralf Thielow
In-Reply-To: <20130212095340.GG2270@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> ... the following fixup is also needed to avoid relying on the shell
> emitting a literal backslash when a backslash isn't followed by a known
> escape character.
>
> -- >8 --
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index cbe36bf..84bd525 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -947,7 +947,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
> test_when_finished "git config --unset core.commentchar" &&
> cat >comment-lines.sh <<EOF &&
> #!$SHELL_PATH
> -sed -e "2,\$ s/^/\\\\\\/" "\$1" >"\$1".tmp
> +sed -e "2,\$ s/^/\\\\\\\\/" "\$1" >"\$1".tmp
> mv "\$1".tmp "\$1"
> EOF
> chmod a+x comment-lines.sh &&
Yeek. If you used write_script with here-text that does not
interpolate,
write_script remove-all-but-the-first.sh <<\EOF
sed -e '2,$s/^/\\/' <"$1" >"$1.tmp" &&
mv "$1.tmp" "$1"
EOF
the above would be much more readable.
I am not sure if I understand what you meant by "literal backslash
blah blah", though.
^ permalink raw reply
* Re: [PATCH] completion: support 'git config --local'
From: Junio C Hamano @ 2013-02-12 17:34 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, Dasa Paddock, Jeff King
In-Reply-To: <1360671642-10272-1-git-send-email-Matthieu.Moy@imag.fr>
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> This needs to be done in two places: __git_config_get_set_variables to
> allow clever completion of "git config --local --get foo<tab>", and
> _git_config to allow "git config --loc<tab>" to complete to --local.
>
> While we're there, change the order of options in the code to match
> git-config.txt.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>> I think this line should include --local:
>>
>> https://github.com/git/git/blob/next/contrib/completion/git-completion.bash#L1782
>> "--global|--system|--file=*)"
>>
>> This would help for:
>> git config -l --local
>
> Yes, but not only ;-)
I see the second hunk is new. Comments?
How would this interract with the writing side of "git config"?
"git config --local foo.bar value" and "git config foo.bar value"
are the same, no?
Is it "yes they are the same but it does not hurt?"
> contrib/completion/git-completion.bash | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 5770b6f..2e1ad67 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1570,7 +1570,7 @@ __git_config_get_set_variables ()
> while [ $c -gt 1 ]; do
> word="${words[c]}"
> case "$word" in
> - --global|--system|--file=*)
> + --system|--global|--local|--file=*)
> config_file="$word"
> break
> ;;
> @@ -1676,7 +1676,7 @@ _git_config ()
> case "$cur" in
> --*)
> __gitcomp "
> - --global --system --file=
> + --system --global --local --file=
> --list --replace-all
> --get --get-all --get-regexp
> --add --unset --unset-all
^ permalink raw reply
* Re: [PATCH v2] rebase -i: respect core.commentchar
From: John Keeping @ 2013-02-12 17:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, git, Ralf Thielow
In-Reply-To: <7v4nhhigp5.fsf@alter.siamese.dyndns.org>
On Tue, Feb 12, 2013 at 09:29:26AM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > ... the following fixup is also needed to avoid relying on the shell
> > emitting a literal backslash when a backslash isn't followed by a known
> > escape character.
> >
> > -- >8 --
> >
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > index cbe36bf..84bd525 100755
> > --- a/t/t3404-rebase-interactive.sh
> > +++ b/t/t3404-rebase-interactive.sh
> > @@ -947,7 +947,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
> > test_when_finished "git config --unset core.commentchar" &&
> > cat >comment-lines.sh <<EOF &&
> > #!$SHELL_PATH
> > -sed -e "2,\$ s/^/\\\\\\/" "\$1" >"\$1".tmp
> > +sed -e "2,\$ s/^/\\\\\\\\/" "\$1" >"\$1".tmp
> > mv "\$1".tmp "\$1"
> > EOF
> > chmod a+x comment-lines.sh &&
>
> Yeek. If you used write_script with here-text that does not
> interpolate,
>
> write_script remove-all-but-the-first.sh <<\EOF
> sed -e '2,$s/^/\\/' <"$1" >"$1.tmp" &&
> mv "$1.tmp" "$1"
> EOF
>
> the above would be much more readable.
Yet another thing for me to learn about ;-)
Do you mean to use that outside the test case, so that the single quotes
work? Or do I still need some level of escaping?
> I am not sure if I understand what you meant by "literal backslash
> blah blah", though.
It turns out that having this in the script works (in bash and dash
although I haven't checked what Posix has to say about it):
sed -e "2,$ s/^/\\\/"
and is equivalent to:
sed -e '2,$ s/^/\\/'
because backslashes that aren't recognised as part of an escape sequence
are not treated specially.
John
^ permalink raw reply
* Re: [PATCH v2] rebase -i: respect core.commentchar
From: Junio C Hamano @ 2013-02-12 18:00 UTC (permalink / raw)
To: John Keeping; +Cc: git, Ralf Thielow
In-Reply-To: <7v4nhhigp5.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> cat >comment-lines.sh <<EOF &&
>> #!$SHELL_PATH
>> -sed -e "2,\$ s/^/\\\\\\/" "\$1" >"\$1".tmp
>> +sed -e "2,\$ s/^/\\\\\\\\/" "\$1" >"\$1".tmp
>> mv "\$1".tmp "\$1"
>> EOF
>> chmod a+x comment-lines.sh &&
>
> Yeek. If you used write_script with here-text that does not
> interpolate,
>
> write_script remove-all-but-the-first.sh <<\EOF
> sed -e '2,$s/^/\\/' <"$1" >"$1.tmp" &&
> mv "$1.tmp" "$1"
> EOF
>
> the above would be much more readable.
As this is already inside a pair of '', we cannot use single quote
around sed expression without doing the ugly '\''.
So it needs to be more like this, and I think it still is more
readable.
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index cbe36bf..8b3e2cd 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -945,13 +945,11 @@ test_expect_success 'rebase -i respects core.commentchar' '
git checkout E^0 &&
git config core.commentchar "\\" &&
test_when_finished "git config --unset core.commentchar" &&
- cat >comment-lines.sh <<EOF &&
-#!$SHELL_PATH
-sed -e "2,\$ s/^/\\\\\\/" "\$1" >"\$1".tmp
-mv "\$1".tmp "\$1"
-EOF
- chmod a+x comment-lines.sh &&
- test_set_editor "$(pwd)/comment-lines.sh" &&
+ write_script remove-all-but-first.sh <<-\EOF &&
+ sed -e "2,\$s/^/\\\\/" "$1" >"$1.tmp" &&
+ mv "$1.tmp" "$1"
+ EOF
+ test_set_editor "$(pwd)/remove-all-but-first.sh" &&
git rebase -i B &&
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
'
^ permalink raw reply related
* Re: [PATCH v4] submodule: add 'deinit' command
From: Junio C Hamano @ 2013-02-12 18:03 UTC (permalink / raw)
To: Phil Hord
Cc: Jens Lehmann, Git Mailing List, Heiko Voigt, Michael J Gruber,
Marc Branchaud, W. Trevor King
In-Reply-To: <CABURp0oQcPotK20QcqCG1pGQPVoa4RnN2nDA=iQoKS99gnPEAQ@mail.gmail.com>
Phil Hord <phil.hord@gmail.com> writes:
>> + if test $# = 0
>> + then
>> + die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
>> + fi
>> +
>> + module_list "$@" |
>> + while read mode sha1 stage sm_path
>> + do
>> + die_if_unmatched "$mode"
>> + name=$(module_name "$sm_path") || exit
>> + url=$(git config submodule."$name".url)
>> + if test -z "$url"
>> + then
>> + say "$(eval_gettext "No url found for submodule path '\$sm_path' in .git/config")"
>
> Is it safe to shelter the user a little bit more from the git
> internals here and say instead:
>
> Submodule '\$sm_path' is not initialized.
Sounds like a sensible suggestion.
> Also, I think this code will show this message for each submodule on
> 'git submodule deinit .' But I think I would prefer to suppress it in
> that case. If I have not explicitly stated which submodules to
> deinit,...
But isn't it the way to explicitly say "everything under the sun"?
After all, what does the message say to "git submodule deinit"?
^ permalink raw reply
* Re: [RFC/PATCH] Replace filepattern with pathspec for consistency
From: Junio C Hamano @ 2013-02-12 18:04 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, Jiang Xin
In-Reply-To: <vpq4nhhv4vc.fsf@grenoble-inp.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> The strings that are fed to _() would be updated with your patch,
>> but the replacement will stay to be "filepattern" translated to the
>> target language. Translators have to actively hunt for the messages
>> to update them. If you left the .po files untouched, they would
>> notice when git.pot is updated the next time and that will ensure
>> that the affected messages get translated, no?
>
> Right, and I'm not competent to say how filepattern was translated in
> any of the languages. Let me know if you want a resend without the po/
> part.
I'll remove the tail part of the patch myself. Thanks.
^ permalink raw reply
* Re: [PATCH v2] rebase -i: respect core.commentchar
From: John Keeping @ 2013-02-12 18:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ralf Thielow
In-Reply-To: <7vvc9xh0p1.fsf@alter.siamese.dyndns.org>
On Tue, Feb 12, 2013 at 10:00:26AM -0800, Junio C Hamano wrote:
>
> So it needs to be more like this, and I think it still is more
> readable.
Agreed. Will you squash this in or do you want a re-roll?
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index cbe36bf..8b3e2cd 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -945,13 +945,11 @@ test_expect_success 'rebase -i respects core.commentchar' '
> git checkout E^0 &&
> git config core.commentchar "\\" &&
> test_when_finished "git config --unset core.commentchar" &&
> - cat >comment-lines.sh <<EOF &&
> -#!$SHELL_PATH
> -sed -e "2,\$ s/^/\\\\\\/" "\$1" >"\$1".tmp
> -mv "\$1".tmp "\$1"
> -EOF
> - chmod a+x comment-lines.sh &&
> - test_set_editor "$(pwd)/comment-lines.sh" &&
> + write_script remove-all-but-first.sh <<-\EOF &&
> + sed -e "2,\$s/^/\\\\/" "$1" >"$1.tmp" &&
> + mv "$1.tmp" "$1"
> + EOF
> + test_set_editor "$(pwd)/remove-all-but-first.sh" &&
> git rebase -i B &&
> test B = $(git cat-file commit HEAD^ | sed -ne \$p)
> '
^ permalink raw reply
* Re: [PATCH v2] rebase -i: respect core.commentchar
From: Junio C Hamano @ 2013-02-12 18:21 UTC (permalink / raw)
To: John Keeping; +Cc: git, Ralf Thielow
In-Reply-To: <20130212175322.GC13501@farnsworth.metanate.com>
John Keeping <john@keeping.me.uk> writes:
>> I am not sure if I understand what you meant by "literal backslash
>> blah blah", though.
>
> It turns out that having this in the script works (in bash and dash
> although I haven't checked what Posix has to say about it):
>
> sed -e "2,$ s/^/\\\/"
>
> and is equivalent to:
>
> sed -e '2,$ s/^/\\/'
>
> because backslashes that aren't recognised as part of an escape sequence
> are not treated specially.
That's POSIX. Inside a dq pair:
\
The <backslash> shall retain its special meaning as an escape
character (see Escape Character (Backslash)) only when followed by
one of the following characters when considered special:
$ ` " \ <newline>
So in your example "\\\/", the first backslash escapes the second
backslash and together they produce a single backslash, the third
backslash is followed by a slash that is not special at all, so it
produces a second backslash, and the slash stands for itself,
resulting in "\\/".
^ permalink raw reply
* Re: [PATCH v2] rebase -i: respect core.commentchar
From: Junio C Hamano @ 2013-02-12 18:23 UTC (permalink / raw)
To: John Keeping; +Cc: git, Ralf Thielow
In-Reply-To: <20130212180917.GD13501@farnsworth.metanate.com>
John Keeping <john@keeping.me.uk> writes:
> On Tue, Feb 12, 2013 at 10:00:26AM -0800, Junio C Hamano wrote:
>>
>> So it needs to be more like this, and I think it still is more
>> readable.
>
> Agreed. Will you squash this in or do you want a re-roll?
I can squash this and the previous one into your original to a
single commit. Thanks.
>
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index cbe36bf..8b3e2cd 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -945,13 +945,11 @@ test_expect_success 'rebase -i respects core.commentchar' '
>> git checkout E^0 &&
>> git config core.commentchar "\\" &&
>> test_when_finished "git config --unset core.commentchar" &&
>> - cat >comment-lines.sh <<EOF &&
>> -#!$SHELL_PATH
>> -sed -e "2,\$ s/^/\\\\\\/" "\$1" >"\$1".tmp
>> -mv "\$1".tmp "\$1"
>> -EOF
>> - chmod a+x comment-lines.sh &&
>> - test_set_editor "$(pwd)/comment-lines.sh" &&
>> + write_script remove-all-but-first.sh <<-\EOF &&
>> + sed -e "2,\$s/^/\\\\/" "$1" >"$1.tmp" &&
>> + mv "$1.tmp" "$1"
>> + EOF
>> + test_set_editor "$(pwd)/remove-all-but-first.sh" &&
>> git rebase -i B &&
>> test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>> '
^ permalink raw reply
* RE: Fetch and -t
From: Olsen, Alan R @ 2013-02-12 18:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <7v621yjmla.fsf@alter.siamese.dyndns.org>
[Sorry for the top-posting. I *hate* Outlook.]
I will need to check why my system is showing old man pages. I am running something compiled from the git tree on kernel.org.
Thanks!
-----Original Message-----
From: Junio C Hamano [mailto:gitster@pobox.com]
Sent: Monday, February 11, 2013 6:25 PM
To: Olsen, Alan R
Cc: git@vger.kernel.org
Subject: Re: Fetch and -t
"Olsen, Alan R" <alan.r.olsen@intel.com> writes:
> I have found that if I add a remote and do a "git fetch -t -f
> remote_name" that it *only* pulls tags.
>
> Reading the man page it seems like it should pull all the remotes and
> all the tags and the commits only reachable by tags.
This is what appears in the documentation we ship these days.
-t::
--tags::
This is a short-hand for giving "refs/tags/*:refs/tags/*"
refspec from the command line, to ask all tags to be fetched
and stored locally. Because this acts as an explicit
refspec, the default refspecs (configured with the
remote.$name.fetch variable) are overridden and not used.
http://git-htmldocs.googlecode.com/git/git-fetch.html
Previous discussion:
http://thread.gmane.org/gmane.comp.version-control.git/180636
A more recent one:
http://thread.gmane.org/gmane.comp.version-control.git/211439/focus=211464
^ permalink raw reply
* Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close
From: Junio C Hamano @ 2013-02-12 18:55 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: peff, git
In-Reply-To: <fc760829f74f31d23f94b61a9e087eda2a66956e.1360677646.git.mina86@mina86.com>
Michal Nazarewicz <mpn@google.com> writes:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> The body of the loop in command_close_bidi_pipe function is identical to
> what _cmd_close function does so instead of duplicating, refactor change
> _cmd_close so that it accepts list of file handlers to be closed, which
s/file handlers/file handles/, I think.
^ permalink raw reply
* Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Junio C Hamano @ 2013-02-12 19:13 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, jrnieder, Brandon Casey
In-Reply-To: <1360664260-11803-6-git-send-email-drafnel@gmail.com>
Brandon Casey <drafnel@gmail.com> writes:
> When 'cherry-pick -s' is used to append a signed-off-by line to a cherry
> picked commit, it does not currently detect the "(cherry picked from..."
> that may have been appended by a previous 'cherry-pick -x' as part of the
> s-o-b footer and it will insert a blank line before appending a new s-o-b.
>
> Let's detect "(cherry picked from...)" as part of the footer so that we
> will produce this:
> ...
> +static int is_cherry_picked_from_line(const char *buf, int len)
> +{
> + /*
> + * We only care that it looks roughly like (cherry picked from ...)
> + */
> + return len > strlen(cherry_picked_prefix) + 1 &&
> + !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
> +}
Does the first "is it longer than the prefix?" check matter? If it
is not, prefixcmp() would not match anyway, no?
^ permalink raw reply
* Re: [PATCH v4 11/12] format-patch: update append_signoff prototype
From: Junio C Hamano @ 2013-02-12 19:29 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, jrnieder, Brandon Casey
In-Reply-To: <1360664260-11803-12-git-send-email-drafnel@gmail.com>
Brandon Casey <drafnel@gmail.com> writes:
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> This is a preparation step for merging with append_signoff from
> sequencer.c
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> Signed-off-by: Brandon Casey <bcasey@nvidia.com>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> builtin/log.c | 13 +------------
> log-tree.c | 17 +++++++++++++----
> revision.h | 2 +-
> 3 files changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 8f0b2e8..59de484 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1086,7 +1086,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> struct commit *origin = NULL, *head = NULL;
> const char *in_reply_to = NULL;
> struct patch_ids ids;
> - char *add_signoff = NULL;
> struct strbuf buf = STRBUF_INIT;
> int use_patch_format = 0;
> int quiet = 0;
> @@ -1193,16 +1192,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> rev.subject_prefix = strbuf_detach(&sprefix, NULL);
> }
>
> - if (do_signoff) {
> - const char *committer;
> - const char *endpos;
> - committer = git_committer_info(IDENT_STRICT);
> - endpos = strchr(committer, '>');
> - if (!endpos)
> - die(_("bogus committer info %s"), committer);
> - add_signoff = xmemdupz(committer, endpos - committer + 1);
> - }
> -
> for (i = 0; i < extra_hdr.nr; i++) {
> strbuf_addstr(&buf, extra_hdr.items[i].string);
> strbuf_addch(&buf, '\n');
> @@ -1393,7 +1382,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> total++;
> start_number--;
> }
> - rev.add_signoff = add_signoff;
> + rev.add_signoff = do_signoff;
> while (0 <= --nr) {
> int shown;
> commit = list[nr];
> diff --git a/log-tree.c b/log-tree.c
> index 5dc45c4..ac1cd68 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -10,6 +10,8 @@
> #include "color.h"
> #include "gpg-interface.h"
>
> +#define APPEND_SIGNOFF_DEDUP (1u <<0)
> +
> struct decoration name_decoration = { "object names" };
>
> enum decoration_type {
> @@ -253,9 +255,12 @@ static int detect_any_signoff(char *letter, int size)
> return seen_head && seen_name;
> }
>
> -static void append_signoff(struct strbuf *sb, const char *signoff)
> +static void append_signoff(struct strbuf *sb, int ignore_footer, unsigned flag)
> {
> + unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
Unused variable at this step?
> static const char signed_off_by[] = "Signed-off-by: ";
> + char *signoff = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
> + getenv("GIT_COMMITTER_EMAIL")));
> size_t signoff_len = strlen(signoff);
> int has_signoff = 0;
> char *cp;
> @@ -275,6 +280,7 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
> if (!isspace(cp[signoff_len]))
> continue;
> /* we already have him */
> + free(signoff);
> return;
> }
>
> @@ -287,6 +293,7 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
> strbuf_addstr(sb, signed_off_by);
> strbuf_add(sb, signoff, signoff_len);
> strbuf_addch(sb, '\n');
> + free(signoff);
> }
>
> static unsigned int digits_in_number(unsigned int number)
> @@ -672,8 +679,10 @@ void show_log(struct rev_info *opt)
> /*
> * And then the pretty-printed message itself
> */
> - if (ctx.need_8bit_cte >= 0)
> - ctx.need_8bit_cte = has_non_ascii(opt->add_signoff);
> + if (ctx.need_8bit_cte >= 0 && opt->add_signoff)
> + ctx.need_8bit_cte =
> + has_non_ascii(fmt_name(getenv("GIT_COMMITTER_NAME"),
> + getenv("GIT_COMMITTER_EMAIL")));
> ctx.date_mode = opt->date_mode;
> ctx.date_mode_explicit = opt->date_mode_explicit;
> ctx.abbrev = opt->diffopt.abbrev;
> @@ -686,7 +695,7 @@ void show_log(struct rev_info *opt)
> pretty_print_commit(&ctx, commit, &msgbuf);
>
> if (opt->add_signoff)
> - append_signoff(&msgbuf, opt->add_signoff);
> + append_signoff(&msgbuf, 0, APPEND_SIGNOFF_DEDUP);
>
> if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
> ctx.notes_message && *ctx.notes_message) {
> diff --git a/revision.h b/revision.h
> index 5da09ee..01bd2b7 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -138,7 +138,7 @@ struct rev_info {
> int reroll_count;
> char *message_id;
> struct string_list *ref_message_ids;
> - const char *add_signoff;
> + int add_signoff;
> const char *extra_headers;
> const char *log_reencode;
> const char *subject_prefix;
^ permalink raw reply
* Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Brandon Casey @ 2013-02-12 19:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Casey, git@vger.kernel.org, pclouds@gmail.com,
jrnieder@gmail.com
In-Reply-To: <7v621xgxax.fsf@alter.siamese.dyndns.org>
On 2/12/2013 11:13 AM, Junio C Hamano wrote:
> Brandon Casey <drafnel@gmail.com> writes:
>
>> When 'cherry-pick -s' is used to append a signed-off-by line to a cherry
>> picked commit, it does not currently detect the "(cherry picked from..."
>> that may have been appended by a previous 'cherry-pick -x' as part of the
>> s-o-b footer and it will insert a blank line before appending a new s-o-b.
>>
>> Let's detect "(cherry picked from...)" as part of the footer so that we
>> will produce this:
>> ...
>> +static int is_cherry_picked_from_line(const char *buf, int len)
>> +{
>> + /*
>> + * We only care that it looks roughly like (cherry picked from ...)
>> + */
>> + return len > strlen(cherry_picked_prefix) + 1 &&
>> + !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>> +}
>
> Does the first "is it longer than the prefix?" check matter? If it
> is not, prefixcmp() would not match anyway, no?
Probably not in practice, but technically we should only be accessing
len characters in buf even though buf may be longer than len. So the
check is just making sure the function doesn't access chars it's not
supposed to.
-Brandon
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply
* Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Junio C Hamano @ 2013-02-12 19:36 UTC (permalink / raw)
To: Brandon Casey
Cc: Brandon Casey, git@vger.kernel.org, pclouds@gmail.com,
jrnieder@gmail.com
In-Reply-To: <511A98C0.70201@nvidia.com>
Brandon Casey <bcasey@nvidia.com> writes:
>>> + return len > strlen(cherry_picked_prefix) + 1 &&
>>> + !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>>> +}
>>
>> Does the first "is it longer than the prefix?" check matter? If it
>> is not, prefixcmp() would not match anyway, no?
>
> Probably not in practice, but technically we should only be accessing
> len characters in buf even though buf may be longer than len. So the
> check is just making sure the function doesn't access chars it's not
> supposed to.
Sorry, I do not follow. Isn't caller's buf terminated with LF at buf[len],
which would never match cherry_picked_prefix even if len is shorter
than the prefix?
^ permalink raw reply
* Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Jonathan Nieder @ 2013-02-12 19:45 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Casey, Brandon Casey, git@vger.kernel.org,
pclouds@gmail.com
In-Reply-To: <511A98C0.70201@nvidia.com>
Brandon Casey wrote:
> On 2/12/2013 11:13 AM, Junio C Hamano wrote:
>> Brandon Casey <drafnel@gmail.com> writes:
>>> +static int is_cherry_picked_from_line(const char *buf, int len)
>>> +{
>>> + /*
>>> + * We only care that it looks roughly like (cherry picked from ...)
>>> + */
>>> + return len > strlen(cherry_picked_prefix) + 1 &&
>>> + !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>>> +}
>>
>> Does the first "is it longer than the prefix?" check matter? If it
>> is not, prefixcmp() would not match anyway, no?
>
> Probably not in practice, but technically we should only be accessing
> len characters in buf even though buf may be longer than len.
Yep. Technically the buf[len - 1] == ')' check is enough to avoid
false positives, but if it and the 'len' check were dropped then this
would be checking that buf is a "(cherry-picked from" line instead of
checking that its first 'len' bytes are one.
So it's just paranoid futureproofing. In the long term, it would be
nice to drop the "number of bytes to ignore at the end" argument to
append_signoff to avoid having to think about this kind of thing.
Jonathan
^ permalink raw reply
* Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Brandon Casey @ 2013-02-12 19:49 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Casey, git@vger.kernel.org, pclouds@gmail.com,
jrnieder@gmail.com
In-Reply-To: <7vtxphfhoq.fsf@alter.siamese.dyndns.org>
On 2/12/2013 11:36 AM, Junio C Hamano wrote:
> Brandon Casey <bcasey@nvidia.com> writes:
>
>>>> + return len > strlen(cherry_picked_prefix) + 1 &&
>>>> + !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>>>> +}
>>>
>>> Does the first "is it longer than the prefix?" check matter? If it
>>> is not, prefixcmp() would not match anyway, no?
>>
>> Probably not in practice, but technically we should only be accessing
>> len characters in buf even though buf may be longer than len. So the
>> check is just making sure the function doesn't access chars it's not
>> supposed to.
>
> Sorry, I do not follow. Isn't caller's buf terminated with LF at buf[len],
> which would never match cherry_picked_prefix even if len is shorter
> than the prefix?
Heh, I almost pointed that out in my reply. Yes, buf will be terminated
with LF at buf[len]. And yes, that means that we will never get a false
positive from prefixcmp even if the comparison overruns buf+len while
doing its comparison. That's why the check doesn't matter in practice,
i.e. based on the way that is_cherry_picked_from_line is being called
right now and the content of cherry_picked_prefix.
But, hasn't is_cherry_picked_from_line entered into a contract with the
caller and said "I will not access more than len characters"?
It's ok with me if you think it reads better without the check.
-Brandon
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply
* Re: [PATCH/FYI v4 13/12] fixup! t/t3511: add some tests of 'cherry-pick -s' functionality
From: Jonathan Nieder @ 2013-02-12 19:56 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, gitster, pclouds
In-Reply-To: <1360664260-11803-14-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> I'm not sure we should apply this though. I'm leaning towards saying that
> the 'cherry-pick -s' behavior with respect to a commit with an empty message
> body should be undefined. If we want it to be undefined then we probably
> shouldn't introduce a test which would have the effect of defining it.
Maybe it would make sense to just check that cherry-pick doesn't
segfault in this case?
That is, compute the output but don't compare it to expected output, as
in:
test_expect_success 'adding signoff to empty message does something sane' '
git reset --hard HEAD^ &&
git cherry-pick --allow-empty-message -s empty-branch &&
git show --pretty=format:%B -s empty-branch >actual &&
# sign-off is included *somewhere*
grep "^Signed-off-by:.*>\$" actual
'
Alternatively, if there are only a few sane behaviors, a test can check
for all of them and pass as long as git follows one. I haven't thought
carefully enough about this example to suggest doing that.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Junio C Hamano @ 2013-02-12 19:58 UTC (permalink / raw)
To: Brandon Casey
Cc: Brandon Casey, git@vger.kernel.org, pclouds@gmail.com,
jrnieder@gmail.com
In-Reply-To: <511A9CDB.9060008@nvidia.com>
Brandon Casey <bcasey@nvidia.com> writes:
> On 2/12/2013 11:36 AM, Junio C Hamano wrote:
>> Brandon Casey <bcasey@nvidia.com> writes:
>>
>>>>> + return len > strlen(cherry_picked_prefix) + 1 &&
>>>>> + !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>>>>> +}
>>>>
>>>> Does the first "is it longer than the prefix?" check matter? If it
>>>> is not, prefixcmp() would not match anyway, no?
>>>
>>> Probably not in practice, but technically we should only be accessing
>>> len characters in buf even though buf may be longer than len. So the
>>> check is just making sure the function doesn't access chars it's not
>>> supposed to.
>>
>> Sorry, I do not follow. Isn't caller's buf terminated with LF at buf[len],
>> which would never match cherry_picked_prefix even if len is shorter
>> than the prefix?
>
> Heh, I almost pointed that out in my reply. Yes, buf will be terminated
> with LF at buf[len]. And yes, that means that we will never get a false
> positive from prefixcmp even if the comparison overruns buf+len while
> doing its comparison. That's why the check doesn't matter in practice,
> i.e. based on the way that is_cherry_picked_from_line is being called
> right now and the content of cherry_picked_prefix.
>
> But, hasn't is_cherry_picked_from_line entered into a contract with the
> caller and said "I will not access more than len characters"?
>
> It's ok with me if you think it reads better without the check.
As Jonathan says, if you rewrite it to
return buf[len - 1] == ')' && !prefixcmp(buf, cherry_picked_prefix);
then the code can keep its promise without the length check, because
it knows there is no ')' in cherry-picked-prefix, and it also knows
prefixcmp() stops at the first difference.
It is not a huge deal; I was primarily reacting to the ugly multi-line
boolean expresion that is not inside a pair of parentheses (and because
this is a "return" statement, there is no good reason to have parentheses
except that this is a multi-line expression), which looked odd.
^ 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