'--tags' for auto push mode to include --follow-tags (pushes annotated tags) by lukasgierth · Pull Request #40 · joseluisq/gitnow

lukasgierth · 2021-08-05T11:52:42Z

Feel free to comment/change.

joseluisq

I think is not bad but we can improve the validation a bit more.

Try what the merge command or other do:
For example this below:

Lines 260 to 292 in 79b9681

    
           set -l v_abort 
        
           set -l v_continue 
        
           set -l v_branch 
        
           for v in $argv 
        
               switch $v 
        
                   case -a --abort 
        
                       set v_abort $v 
        
                   case -c --continue 
        
                       set v_continue $v 
        
                   case -h --help 
        
                       echo "NAME" 
        
                       echo "      Gitnow: merge - Merge given branch into the active one" 
        
                       echo "EXAMPLES" 
        
                       echo "      merge <branch to merge>" 
        
                       echo "OPTIONS:" 
        
                       echo "      -a --abort              Abort a conflicted merge" 
        
                       echo "      -c --continue           Continue a conflicted merge" 
        
                       echo "      -h --help               Show information about the options for this command" 
        
                       return 
        
                   case -\* 
        
                   case '*' 
        
                       set v_branch $v 
        
               end 
        
           end 
        
           # abort 
        
           if test "$v_abort"; 
        
               echo "Abort the current merge" 
        
               command git merge --abort 
        
               commandline -f repaint 
        
               return 
        
           end

Can you follow the code?
What essentially does is it just iterate all possible args passed for that command and collect them inside a loop and a switch. Then finally verify some options as needed.
The idea of this approach is that not always a particular flag like --tags can be in the same array position.
For example in your code you are assuming that --tags is coming in the second item (index 1) of the $opts array which can be not necessarily the case depending in the user typing.

joseluisq · 2021-08-05T12:21:45Z

I think that loop and switch approach give you more flexibility to decide when apply certain actions independently of user typing. It also increases readability and you could add a --help flag support even.

joseluisq · 2021-08-05T12:24:59Z

Also those are the "old" commands pull and push that need that flags checking improvement.

lukasgierth · 2021-08-05T12:44:20Z

The question is: What happens to/with the auto mode? Those other commands (merge/move) work that way because they have no auto mode.
If i implement it like that, how do we decide to use auto/manual mode?
When '--tags' is given as an option, do i ignore all other args and use auto mode + '--follow-tags'?

joseluisq

I did upgrade the draft code.

conf.d/gitnow.fish

lukasgierth · 2021-08-05T13:23:43Z

Did change it, was already implementing it.

joseluisq

I think we need to adjust a bit the push command.
Here my tests:

# 0.
~/d/test (master) 18ms > tag
v0.0.0
v0.0.0-beta.4
v0.0.0-beta.3
v0.0.0-beta.2
v0.0.0-beta.1
v0.0.0-beta.0

# 1.
~/d/test (master) 25ms > push -h
🚀 Pushing changes...
NAME
      Gitnow: push - Push current branch to default origin
OPTIONS:
      -t --tags               (auto mode) include annotated tags the relate to the commits
      -h --help               Show information about the options for this command

# 2.
~/d/test (master) 26ms > push -t
🚀 Pushing changes...
Mode: Auto (incl. tags)
Remote URL: origin (ssh://git@server.com/test/test.git)
Remote branch: master

Enumerating objects: 3, done.
Counting objects: 100% (3/3), done.
Delta compression using up to 4 threads
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 894 bytes | 894.00 KiB/s, done.
Total 3 (delta 0), reused 0 (delta 0), pack-reused 0
To server.com:test/test.git
 * [new branch]      master -> master
Branch 'master' set up to track remote branch 'master' from 'origin'.

# 3.
~/d/test (master) 995ms > git push origin master --follow-tags
Everything up-to-date

Review

This one is fine but the echo 🚀 Pushing changes... is wrong.
This looks ok at the beginning but at the end the tags are not pushed. See next one.
Looks like --follow-tags pushes annotated tags only. That's why we should prefer just the --tags flag similar to pull command.

Here using --tags for pushing:

~/d/test (master) 4.95s > git push origin master --tags
Enumerating objects: 3, done.
Counting objects: 100% (3/3), done.
Delta compression using up to 4 threads
Compressing objects: 100% (2/2), done.
Writing objects: 100% (2/2), 904 bytes | 904.00 KiB/s, done.
Total 2 (delta 1), reused 0 (delta 0), pack-reused 0
To server.com:test/test.git
   221c82f..bfacee4  master -> master
 * [new tag]         v0.0.0-beta.5 -> v0.0.0-beta.5
 * [new tag]         v0.0.0-beta.6 -> v0.0.0-beta.6

lukasgierth · 2021-08-05T15:23:13Z

Regarding 3), do we want that? I mean pushing all tags, because that's what it does as far as i understand. --follow-tags does only push the annotated tags, which relate to the commits I am pushing.

joseluisq

Regarding 3), do we want that? I mean pushing all tags, because that's what it does as far as i understand. --follow-tags does only push the annotated tags, which relate to the commits I am pushing.

Sorry, I misunderstood the intention of the PR. You are right about --follow-tags.
I think for make it even more clear please update the PR description accordingly.

conf.d/gitnow.fish

joseluisq · 2021-08-05T15:32:34Z

After those changes I think we should be ready to merge.

lukasgierth · 2021-08-05T15:42:56Z

I think it makes sense (for now) to only push the annotated tags with the extended auto mode since these are meant to be for releases and the lightweight tags without annotations are made for local use only. Those should not be pushed so easily, at least not in auto mode.

joseluisq

I think it makes sense (for now) to only push the annotated tags with the extended auto mode since these are meant to be for releases and the lightweight tags without annotations are made for local use only. Those should not be pushed so easily, at least not in auto mode.

Yeah, that was the idea so we are done.

conf.d/gitnow.fish

joseluisq · 2021-08-05T15:45:57Z

Thanks 👍

'push --tags' mode to include --follow-tags in push auto mode

b63e0e8

joseluisq self-assigned this Aug 5, 2021

joseluisq added the enhancement New feature or request label Aug 5, 2021

joseluisq requested changes Aug 5, 2021

View reviewed changes

joseluisq reviewed Aug 5, 2021

View reviewed changes

conf.d/gitnow.fish Outdated Show resolved Hide resolved

lukasgierth added 2 commits August 5, 2021 15:17

cleanup: push auto/manual with switch

919c675

fix: reinclude echo and linebreaks

4a19413

lukasgierth requested a review from joseluisq August 5, 2021 13:24

joseluisq requested changes Aug 5, 2021

View reviewed changes

conf.d/gitnow.fish Outdated Show resolved Hide resolved

lukasgierth added 3 commits August 5, 2021 17:33

push: echo at the correct place

34d4f17

cleanup: remove commented part which is not used

7963e6d

fix: typo

6ac5c6b

lukasgierth changed the title ~~'push --tags' mode to include --follow-tags in push auto mode~~ '--tags' for auto push mode to include --follow-tags (pushed annotated tags) Aug 5, 2021

lukasgierth changed the title ~~'--tags' for auto push mode to include --follow-tags (pushed annotated tags)~~ '--tags' for auto push mode to include --follow-tags (pushes annotated tags) Aug 5, 2021

joseluisq reviewed Aug 5, 2021

View reviewed changes

conf.d/gitnow.fish Outdated Show resolved Hide resolved

joseluisq approved these changes Aug 5, 2021

View reviewed changes

joseluisq merged commit 0a87d53 into joseluisq:master Aug 5, 2021

	set -l v_abort
	set -l v_continue
	set -l v_branch

	for v in $argv
	switch $v
	case -a --abort
	set v_abort $v
	case -c --continue
	set v_continue $v
	case -h --help
	echo "NAME"
	echo " Gitnow: merge - Merge given branch into the active one"
	echo "EXAMPLES"
	echo " merge <branch to merge>"
	echo "OPTIONS:"
	echo " -a --abort Abort a conflicted merge"
	echo " -c --continue Continue a conflicted merge"
	echo " -h --help Show information about the options for this command"
	return
	case -\*
	case '*'
	set v_branch $v
	end
	end

	# abort
	if test "$v_abort";
	echo "Abort the current merge"
	command git merge --abort
	commandline -f repaint
	return
	end

Uh oh!

Conversation

lukasgierth commented Aug 5, 2021

Uh oh!

joseluisq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joseluisq commented Aug 5, 2021

Uh oh!

joseluisq commented Aug 5, 2021

Uh oh!

lukasgierth commented Aug 5, 2021

Uh oh!

joseluisq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukasgierth commented Aug 5, 2021

Uh oh!

joseluisq left a comment

Choose a reason for hiding this comment

Uh oh!

lukasgierth commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joseluisq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joseluisq commented Aug 5, 2021

Uh oh!

lukasgierth commented Aug 5, 2021

Uh oh!

joseluisq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joseluisq commented Aug 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joseluisq left a comment •

edited

Loading

lukasgierth commented Aug 5, 2021 •

edited

Loading