Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
Remi Galan Alfonso writes: > Junio C Hamano writes: >> I think there is a difference between (silently) accepting just to >> be lenient and documenting and advocating mixed case uses. >> >> Personally, I'd rather not to see gratuitous flexibility to allow >> the same thing spelled in 47 different ways for no good reason. > > It was more of a mistake on our part rather than actually wanting to > document mixed case uses. > > In the v2 of the patch (not sent to the mailing list yet since we want > to take into consideration the conclusion of this discussion before) > it is entirely in lower case in both the documentation and the code > while we silently allow upper and mixed case. Understood; I am not sold on the whole "warning" business, though. I think I saw you did 'tr [:upper:]' or something like that in the patch; we tend to avoid [:class:] and [=equiv=] when not needed, unless we know that the matching engine used supports them (i.e. it is OK to use them in Perl scripts and it is OK to feed them to the wildmatch-based matcher in Git itself, but not in general shell scripts). As the values can all be represented in US-ASCII, it should be sufficient to do "tr 'A-Z' 'a-z'", I would think. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
Junio C Hamano writes: > I think there is a difference between (silently) accepting just to > be lenient and documenting and advocating mixed case uses. > > Personally, I'd rather not to see gratuitous flexibility to allow > the same thing spelled in 47 different ways for no good reason. It was more of a mistake on our part rather than actually wanting to document mixed case uses. In the v2 of the patch (not sent to the mailing list yet since we want to take into consideration the conclusion of this discussion before) it is entirely in lower case in both the documentation and the code while we silently allow upper and mixed case. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
Matthieu Moy writes: > Stephen Kelly writes: > >> Galan Rémi ensimag.grenoble-inp.fr> writes: >> >>> >>> Check if commits were removed (i.e. a line was deleted) or dupplicated >>> (e.g. the same commit is picked twice), can print warnings or abort >>> git rebase according to the value of the configuration variable >>> rebase.checkLevel. >> >> I sometimes duplicate commits deliberately if I want to split a commit in >> two. I move a copy up and fix the conflict, and I know that I'll still get >> the right thing later even if I make a mistake with the conflict >> resolution. > > The more I think about it, the more I think we should either not warn at > all on duplicate commits, or have a separate config variable. Yeah, I'd say we shouldn't warn, without configuration to keep things simple. > > It's rare to duplicate by mistake, and when you do so, it's already easy > to notice: you get conflicts, and you can git rebase --skip the second > occurence. Accidentally dropped commits are another story: it's rather > easy to cut-and-forget-to-paste, and the consequence currently is silent > data loss ... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
Remi Galan Alfonso
writes:
> Thank you for reviewing the code.
>
> Eric Sunshine writes:
>> > + # To uppercase
>> > + checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')
>>
>> Is there precedence elsewhere for recognizing uppercase and lowercase
>> variants of config values?
>
> It seems to be commonly used when parsing options in the C files
> through strcasecmp. For exemple, in config.c:818 :
> if (!strcmp(var, "core.safecrlf")) {
> if (value && !strcasecmp(value, "warn")) {
> [...]
> However we didn't see any precedence in shell files. Do you think we
> should remove it?
I think there is a difference between (silently) accepting just to
be lenient and documenting and advocating mixed case uses.
Personally, I'd rather not to see gratuitous flexibility to allow
the same thing spelled in 47 different ways for no good reason.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
On Wed, May 27, 2015 at 9:19 AM, Remi Galan Alfonso
wrote:
> Eric Sunshine writes:
>> > + # To uppercase
>> > + checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')
>>
>> Is there precedence elsewhere for recognizing uppercase and lowercase
>> variants of config values?
>
> It seems to be commonly used when parsing options in the C files
> through strcasecmp. For exemple, in config.c:818 :
> if (!strcmp(var, "core.safecrlf")) {
> if (value && !strcasecmp(value, "warn")) {
> [...]
> However we didn't see any precedence in shell files. Do you think we
> should remove it?
Precedence in C code is good enough for me, and it makes sense for
your new code to follow suit by being insensitive to case (as you have
already done).
However, it would be a good idea to be consistent in your use of
uppercase/lowercase in the commit message, documentation, and code,
rather than having a mix. I'd suggest sticking with lowercase
throughout since lowercase is more commonly used in the codebase (and
just easier to read).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
Eric Sunshine writes: > Shouldn't this case also 'die' when rebase.checkLevel is "error"? And, > why doesn't the user get advice about configuring rebase.checkLevel in > this case? Stephen Kelly writes: > I sometimes duplicate commits deliberately if I want to split a commit in > two. Matthieu Moy writes: > The more I think about it, the more I think we should either not warn at > all on duplicate commits, or have a separate config variable. Put in common because two config variables would have an effect on the 'die' and advise part. In this patch we didn't put the 'die' in the duplicate commit part since there was only one config variable and there are cases where the user might want to duplicate commits. After the code reviewing of Eric Sunshine and Stephen Kelly, we also came to the conclusion that we should use two config variables, one about missing commits and the other about duplicate commits. This way if you deliberately want to use duplicate commits, you can just set the value to 'ignore' for duplicate commits and still have 'warn'/'error' for missing commits. Moreover, each part would have its 'die' depending on the value of the corresponding config variable. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
Thank you for reviewing the code.
Eric Sunshine writes:
> > + # To uppercase
> > + checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')
>
> Is there precedence elsewhere for recognizing uppercase and lowercase
> variants of config values?
It seems to be commonly used when parsing options in the C files
through strcasecmp. For exemple, in config.c:818 :
if (!strcmp(var, "core.safecrlf")) {
if (value && !strcasecmp(value, "warn")) {
[...]
However we didn't see any precedence in shell files. Do you think we
should remove it?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
Stephen Kelly writes: > Galan Rémi ensimag.grenoble-inp.fr> writes: > >> >> Check if commits were removed (i.e. a line was deleted) or dupplicated >> (e.g. the same commit is picked twice), can print warnings or abort >> git rebase according to the value of the configuration variable >> rebase.checkLevel. > > I sometimes duplicate commits deliberately if I want to split a commit in > two. I move a copy up and fix the conflict, and I know that I'll still get > the right thing later even if I make a mistake with the conflict > resolution. The more I think about it, the more I think we should either not warn at all on duplicate commits, or have a separate config variable. It's rare to duplicate by mistake, and when you do so, it's already easy to notice: you get conflicts, and you can git rebase --skip the second occurence. Accidentally dropped commits are another story: it's rather easy to cut-and-forget-to-paste, and the consequence currently is silent data loss ... -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
Galan Rémi ensimag.grenoble-inp.fr> writes: > > Check if commits were removed (i.e. a line was deleted) or dupplicated > (e.g. the same commit is picked twice), can print warnings or abort > git rebase according to the value of the configuration variable > rebase.checkLevel. I sometimes duplicate commits deliberately if I want to split a commit in two. I move a copy up and fix the conflict, and I know that I'll still get the right thing later even if I make a mistake with the conflict resolution.
Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
On Tue, May 26, 2015 at 5:38 PM, Galan Rémi
wrote:
> git rebase -i: Warn removed or dupplicated commits
s/dupplicated/duplicated/
Also, drop capitalization, and insert "about":
git rebase -i: warn about removed or duplicated commits
> Check if commits were removed (i.e. a line was deleted) or dupplicated
s/dupplicated/duplicated/
> (e.g. the same commit is picked twice), can print warnings or abort
s/can/and/, I think.
> git rebase according to the value of the configuration variable
> rebase.checkLevel.
>
> Add the configuration variable rebase.checkLevel.
> - When unset or set to "IGNORED", no checking is done.
s/IGNORED/IGNORE/
> - When set to "WARN", the commits are checked, warnings are
> displayed but git rebase still proceeds.
> - When set to "ERROR", the commits are checked, warnings are
> displayed and the rebase is aborted.
Why uppercase for these names? Is there precedence for that? I think
lowercase is more common.
> Signed-off-by: Galan Rémi
> ---
> This part of the patch has no test yet, it is more for rfc.
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d44bc85..2152e27 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2204,6 +2204,14 @@ rebase.autoStash::
> successful rebase might result in non-trivial conflicts.
> Defaults to false.
>
> +rebase.checkLevel::
> + If set to "warn", git rebase -i will print a warning if some
> + commits are removed (i.e. a line was deleted) or if some
> + commits appear more than one time (e.g. the same commit is
> + picked twice), however the rebase will still proceed. If set
> + to "error", it will print the previous warnings and abort the
> + rebase.
The commit message talks about "ignore", but there is no mention here.
Also, what is the default behavior if not specified? That should be documented.
Finally, this talks about lowercase "warn" and "error", whereas the
commit message uses upper case "WARN" and "ERROR", as does the code.
Why the inconsistency?
> receive.advertiseAtomic::
> By default, git-receive-pack will advertise the atomic push
> capability to its clients. If you don't want to this capability
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 3cd2ef2..cb05cbb 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -213,6 +213,11 @@ rebase.autoSquash::
> rebase.autoStash::
> If set to true enable '--autostash' option by default.
>
> +rebase.checkLevel::
> + If set to "warn" print warnings about removed commits and
> + duplicated commits in interactive mode. If set to "error"
> + print the warnings and abort the rebase. No check by default.
Ditto: Fails to mention "ignore".
> OPTIONS
> ---
> --onto ::
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index cb749e8..8a837ca 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -837,6 +837,80 @@ add_exec_commands () {
> mv "$1.new" "$1"
> }
>
> +# Print the list of the sha-1 of the commits
> +# from a todo list in a file.
> +# $1 : todo-file, $2 : outfile
> +todo_list_to_sha_list () {
> + todo_list=$(git stripspace --strip-comments < "$1")
> + temp_file=$(mktemp)
> + echo "$todo_list" > "$temp_file"
> + while read -r command sha1 rest < "$temp_file"
On this project it is typical to drop the space after redirection
operators (<, >, >>), however, git-rebase--interactive.sh is filled
with both styles (space and no space after redirection). New code
probably ought to drop the space.
> + do
> + case "$command" in
> + x|"exec")
> + ;;
> + *)
> + echo "$sha1" >> "$2"
> + ;;
> + esac
> + sed -i '1d' "$temp_file"
> + done
> + rm "$temp_file"
> +}
> +
> +# Check if the user dropped some commits by mistake
> +# or if there are two identical commits.
> +# Behaviour determined by .gitconfig.
> +check_commits () {
> + checkLevel=$(git config --get rebase.checkLevel)
> + checkLevel=${checkLevel:-"IGNORE"}
Minor aside: Unnecessary quoting increases the noise level, thus
making the code slightly more difficult to read. This could just as
well have been:
checkLevel=${checkLevel:-IGNORE}
There are plenty of other places throughout this patch which exhibit
the same shortcoming, but I won't point them out individually.
> + # To uppercase
> + checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')
Is there precedence elsewhere for recognizing uppercase and lowercase
variants of config values?
> + case "$checkLevel" in
> + "WARN"|"ERROR")
> + todo_list_to_sha_list "$todo".backup "$todo".oldsha1
> + todo_list_to_sha_list "$todo" "$todo".n
[PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
Check if commits were removed (i.e. a line was deleted) or dupplicated
(e.g. the same commit is picked twice), can print warnings or abort
git rebase according to the value of the configuration variable
rebase.checkLevel.
Add the configuration variable rebase.checkLevel.
- When unset or set to "IGNORED", no checking is done.
- When set to "WARN", the commits are checked, warnings are
displayed but git rebase still proceeds.
- When set to "ERROR", the commits are checked, warnings are
displayed and the rebase is aborted.
Signed-off-by: Galan Rémi
---
This part of the patch has no test yet, it is more for rfc.
Documentation/config.txt | 8 +
Documentation/git-rebase.txt | 5 +++
git-rebase--interactive.sh | 76
3 files changed, 89 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index d44bc85..2152e27 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2204,6 +2204,14 @@ rebase.autoStash::
successful rebase might result in non-trivial conflicts.
Defaults to false.
+rebase.checkLevel::
+ If set to "warn", git rebase -i will print a warning if some
+ commits are removed (i.e. a line was deleted) or if some
+ commits appear more than one time (e.g. the same commit is
+ picked twice), however the rebase will still proceed. If set
+ to "error", it will print the previous warnings and abort the
+ rebase.
+
receive.advertiseAtomic::
By default, git-receive-pack will advertise the atomic push
capability to its clients. If you don't want to this capability
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3cd2ef2..cb05cbb 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -213,6 +213,11 @@ rebase.autoSquash::
rebase.autoStash::
If set to true enable '--autostash' option by default.
+rebase.checkLevel::
+ If set to "warn" print warnings about removed commits and
+ duplicated commits in interactive mode. If set to "error"
+ print the warnings and abort the rebase. No check by default.
+
OPTIONS
---
--onto ::
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index cb749e8..8a837ca 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -837,6 +837,80 @@ add_exec_commands () {
mv "$1.new" "$1"
}
+# Print the list of the sha-1 of the commits
+# from a todo list in a file.
+# $1 : todo-file, $2 : outfile
+todo_list_to_sha_list () {
+ todo_list=$(git stripspace --strip-comments < "$1")
+ temp_file=$(mktemp)
+ echo "$todo_list" > "$temp_file"
+ while read -r command sha1 rest < "$temp_file"
+ do
+ case "$command" in
+ x|"exec")
+ ;;
+ *)
+ echo "$sha1" >> "$2"
+ ;;
+ esac
+ sed -i '1d' "$temp_file"
+ done
+ rm "$temp_file"
+}
+
+# Check if the user dropped some commits by mistake
+# or if there are two identical commits.
+# Behaviour determined by .gitconfig.
+check_commits () {
+ checkLevel=$(git config --get rebase.checkLevel)
+ checkLevel=${checkLevel:-"IGNORE"}
+ # To uppercase
+ checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')
+
+ case "$checkLevel" in
+ "WARN"|"ERROR")
+ todo_list_to_sha_list "$todo".backup "$todo".oldsha1
+ todo_list_to_sha_list "$todo" "$todo".newsha1
+
+ duplicates=$(sort "$todo".newsha1 | uniq -d)
+
+ echo "$(sort -u "$todo".oldsha1)" > "$todo".oldsha1
+ echo "$(sort -u "$todo".newsha1)" > "$todo".newsha1
+ missing=$(comm -2 -3 "$todo".oldsha1 "$todo".newsha1)
+
+ # check missing commits
+ if ! test -z "$missing"
+ then
+ warn "Warning : some commits may have been dropped
accidentally."
+ warn "Dropped commits:"
+ warn "$missing"
+ warn "To avoid this message, use \"drop\" to
explicitely remove a commit."
+ warn "Use git --config rebase.checkLevel to change"
+ warn "the level of warnings (ignore,warn,error)."
+ warn ""
+
+ if test "$checkLevel" = "ERROR"
+ then
+ die_abort "Rebase aborted due to dropped
commits."
+ fi
+ fi
+
+ # check duplicate commits
+ if ! test -z "$duplicates"
+ then
+ warn "Warning : some commits have been used twice:"
+ warn "$duplicates"
+ warn ""
+ fi
+ ;;
+ "IGNORE")
+
