Re: [PATCH 3/4] status: give more information during rebase -i

2015-06-09 Thread Guillaume Pages
Junio C Hamano gits...@pobox.com writes:

Guillaume Pagès guillaume.pa...@ensimag.grenoble-inp.fr writes: 

 git status gives more information during rebase -i, about the list of 
 command that are done during the rebase. It displays the two last 
 commands executed and the two next lines to be executed. It also gives 
 hints to find the whole files in .git directory. 
 --- 

Without 1/4 and 2/4 in the same thread, it is hard to guess what you 
wanted to do with these two patches. Remember, reviewers review not 
just your patches but those from many others. 

I would in the meantime assume these are replacement patches for the 
ones in 

http://thread.gmane.org/gmane.comp.version-control.git/270743/focus=270744 

You assumed correctly, I didn't wanted to flood the mailing list and I
assumed it would be linked correctly with the previous since. Looks like
I was wrong.

 diff --git a/wt-status.c b/wt-status.c 
 index c83eca5..7f88470 100644 
 --- a/wt-status.c 
 +++ b/wt-status.c 
 @@ -1026,12 +1026,73 @@ static int split_commit_in_progress(struct wt_status 
 *s) 
 return split_in_progress; 
 } 
 
 +static void show_rebase_information(struct wt_status *s, 
 + struct wt_status_state *state, 
 + const char *color) 
 +{ 
 + if (state-rebase_interactive_in_progress) { 
 + int i, begin; 
 + int lines_to_show_nr = 2; 

lines_to_show = 2 or nr_lines_to_show = 2 would be easier to read. 

Done

 + 
 + struct strbuf buf = STRBUF_INIT; 
 + struct string_list have_done = STRING_LIST_INIT_DUP; 
 + struct string_list yet_to_do = STRING_LIST_INIT_DUP; 
 + 
 + strbuf_read_file(buf, git_path(rebase-merge/done), 0); 
 + stripspace(buf, 1); 
 + have_done.nr = string_list_split(have_done, buf.buf, '\n', -1); 
 + string_list_remove_empty_items(have_done, 1); 
 + strbuf_release(buf); 
 + 
 + strbuf_read_file(buf, git_path(rebase-merge/git-rebase-todo), 0); 
 + stripspace(buf, 1); 
 + string_list_split(yet_to_do, buf.buf, '\n', -1); 
 + string_list_remove_empty_items(yet_to_do, 1); 
 + strbuf_release(buf); 
 + 
 + if (have_done.nr == 0) 
 + status_printf_ln(s, color, _(No commands done.)); 

Do the users even need to be told that, I wonder? 

I guess it removes the ambiguity of being told nothing.

 + else{ 

Style: else { 

Ok thanks.

 + status_printf_ln(s, color, 
 + Q_(Last command done (%d command done):, 
 + Last commands done (%d commands done):, 
 + have_done.nr), 
 + have_done.nr); 
 + begin = (have_done.nr  lines_to_show_nr) ? have_done.nr-lines_to_show_nr 
 : 0; 
 + for (i = begin; i  have_done.nr; i++) { 
 + status_printf_ln(s, color,  %s, have_done.items[i].string); 
 + } 

Hmm, perhaps fold lines like this (and you do not need begin)? 

for (i = (lines_to_show_nr  have_done.nr) 
? have_done.nr - lines_to_show_nr : 0; 
i  have_done.nr; 
i++) 
status_printf_ln(...);



 + if (have_done.nr  lines_to_show_nr  s-hints) 
 + status_printf_ln(s, color, 
 + _( (see more in file %s)), git_path(rebase-merge/done)); 

That's a nice touch ;-) 

 + } 
 + if (yet_to_do.nr == 0) 
 + status_printf_ln(s, color, 
 + _(No commands remaining.)); 

This I can see why we may want to say it. 

 + else{ 
 + 
 + status_printf_ln(s, color, 
 + Q_(Next command to do (%d remaining command):, 
 + Next commands to do (%d remaining commands):, 
 + yet_to_do.nr), 
 + yet_to_do.nr); 
 + for (i = 0; i  lines_to_show_nr  i  yet_to_do.nr; i++) { 
 + status_printf_ln(s, color,  %s, yet_to_do.items[i].string); 
 + } 
 + if (s-hints) 
 + status_printf_ln(s, color, 
 + _( (use \git rebase --edit-todo\ to view and edit))); 
 + } 

Make sure you do not leak memory used by two string lists here... 

Done, thanks again.





--
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 1/4] status: factor two rebase-related messages together

2015-06-09 Thread Guillaume Pages
Junio C Hamano gits...@pobox.com writes:

Hmmm, it obviously does not break anything but it is not obvious why 
this is a good change. 

Is it that you wanted to have a single instance of if on a branch, 
we say 'you are rebasing that branch', otherwise we say 'you are 
rebasing'? Even then, I am not sure if this code movement was the 
best way to do so (an obvious alternative is to use a shared helper 
function and call from the two arms of if/elseif/... chain). 

I made this change because at first sight, this piece of code was 
difficult to read for me. There was two long branches very similar
and I had to spot the differences, and the actual differences were
at the very end of the branches so I had to check back what the
condition was about. It seems now much more natural to me: the part
in common of both branches is in OR-condition and the differences 
between branches are gathered with the test on the variable they
depend.

By the way, I agree that this change is not absolutely necessary.
--
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 3/4] status: give more information during rebase -i

2015-06-05 Thread Guillaume Pages
Junio C Hamano gits...@pobox.com writes

Matthieu Moy matthieu@grenoble-inp.fr writes: 

 +void get_two_last_lines(char *filename, int *numlines, char **lines) 
 +{ 
 +... 
 +} 
 + 
 +void get_two_first_lines(char *filename, int *numlines, char **lines) 
 +{ 
 +... 
 +} 

I had a handful of comments on these: 

- Do we need two separate and overly specific functions like these, 
i.e. two and first/last? 

- Wouldn't people want to be able to configure the number of lines? 

- Do we really want get_two_{first,last}_LINES() functions? 

I am wondering if insn sheets these functions read include 
comments, in which case get_n_{first,last}_commands() may be a 
more correct name. 

- Wouldn't it be necessary for these functions to report the total 
number of commands, instead of giving void back? Otherwise how 
would the caller produce summary like this: 

I felt that was not the right way to do so. What do you think of a
function like that:

/*
 * Puts nb_commands commands from filename in lines,
 * returns the total number of commands in the file
 * ignores comments and empty lines
 * lines needs to be at least of size nb_commands
 * part: 0 get last commands
 *   1 get first commands
 */

int get_commands(char *filename, int nb_commands, char **lines, int part)

Maybe part is not the best word to choose to take the beginning or the end
of the file. I also hesitate about adding a parameter to ignore or not the 
comments.

--
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: [RFC/PATCH 2/2] status: fix tests to handle new verbose git status during rebase

2015-06-03 Thread Guillaume Pages
Thanks for reviewing, I take everything into account and release a v2 ASAP.

Guillaume
--
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


[BUG] potential error in parsing git checkout options?

2015-05-29 Thread Guillaume Pages
Git version : 2.4.1.652.g8fd8657 

When we run git checkout -babar, we would expect an error message like 
unknown switch 'a'  and we get Switched to a new branch 'abar'. We 
are not sure since we don't entirely understand the syntax -b branch 
which is shown in the documentation, but as average users, we found it 
a bit surprising. 


Guillaume Pagès
--
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: [RFC/PATCH v2] create a skeleton for the command git rebase --status

2015-05-29 Thread Guillaume Pages
After this discussion I eventually agree that it would be better
upgrading git status than creating a new command.When people use git
status, it means that they need information to continue their work, so
if you don't even know that you are in a rebase, you will very likely
need information about the current rebase.

During a classic rebase we could have output like:

rebase in progress; onto d9d448a You are currently rebasing branch
'branche1' on 'd9d448a'.  (fix conflicts and then run git rebase
--continue) (use git rebase --skip to skip this patch) (use git
rebase --abort to check out the original branch) (5 commits applied,
3 remainings) Failed to apply:

252c273 commit message

Unmerged paths: (use git reset HEAD file... to unstage) (use git
add file... to mark resolution)

both modified: file1


And during an interactive rebase:

rebase in progress; onto 247c883 You are currently editing a commit
while rebasing branch 'b1.1' on '247c883'.  (use git commit --amend
to amend the current commit) (use git rebase --continue once you are
satisfied with your changes)

Last commands done (5 commands done) :

pick 62609785 commit message1 reword 85ae9001 new commit message2

(See more at .git/rebase-merge/done)

Next commands to do (3 remainings commands) :

squash 62609785 commit message3 pick 85ae9001 commit message4

(See more at .git/rebase-merge/git-rebase-todo)

Changes not staged for commit: (use git add file... to update what
will be committed) (use git checkout -- file... to discard changes
in working directory)

modified: file1 ...

Is it a good practice to send the user find information in the .git
directory?

Thanks

Guillaume
--
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: Implementation of git rebase --status

2015-05-26 Thread Guillaume Pages

Junio C Hamano gits...@pobox.com writes:

Guillaume Pages guillaume.pa...@ensimag.grenoble-inp.fr writes: 

 Do you think it could be useful or do you have any suggestion? 

All of your examples say things like: 

 You are in the middle of a rebase session. 
 The line that paused this session is: 

but what if there is no such line? 

IOW, what does the user see when using this new option during a git 
rebase (not git rebase -i)? 

I guess it should display the sha1 of the patch that failed in this case.

Other than that, sounds like a neat thing to do. 
--
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


Implementation of git rebase --status

2015-05-26 Thread Guillaume Pages
Hi, 

I would like to implement a new command git rebase --status to inform the user 
about the current rebase session. Here is a sample of what I think it could 
look like in case of merge conflict: 

git rebase --status 
You are in the middle of a rebase session. 
The line that paused this session is: 
pick 848a16f commit with conflicts 
CONFLICT (content): Merge conflict in file1 
Consult and edit remaining actions with git rebase --edit-todo 

In case of syntax error: 

git rebase --status 
You are in the middle of a rebase session. 
The line that paused this session is: 
tick 848a16f syntax error 
SYNTAX ERROR 
Consult and edit remaining actions with git rebase --edit-todo 

In case of error during the execution of a script: 

git rebase --status 
You are in the middle of a rebase session. 
The line that paused this session is: 
exec exit 3 
ERROR IN SCRIPT EXECUTION 
Consult and edit remaining actions with git rebase --edit-todo 

Do you think it could be usefull or do you have any suggestion? 

Thanks, 

Guillaume Pagès 
--
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